Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier,
On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
> Some DRM drivers check the vgacon_text_force() function return value as an
> indication on whether they should be allowed to be enabled or not.
>
> This function returns true if the nomodeset kernel command line parameter
> was set. But there may be other conditions besides this to determine if a
> driver should be enabled.
>
> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
> can be later extended if needed, without having to modify all the drivers.
>
> Also, while being there do some cleanup. The vgacon_text_force() function
> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>
> Suggested-by: Thomas Zimmermann
> Signed-off-by: Javier Martinez Canillas
> ---
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..3fb567d62881 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const
> char *name)
> }
> EXPORT_SYMBOL(drm_dev_set_unique);
>
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> + if (vgacon_text_force()) {
> + DRM_INFO("%s driver is disabled\n", driver->name);
DRM_INFO is deprecated, please do not use it in new code.
Also other users had an error message and not just info - is info
enough?
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
> /*
> * DRM Core
> * The DRM core module initializes all global DRM objects and makes them
> diff --git a/drivers/gpu/drm/i915/i915_module.c
> b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..45cb3e540eff 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -18,9 +18,12 @@
> #include "i915_selftest.h"
> #include "i915_vma.h"
>
> +static const struct drm_driver driver;
Hmmm...
> +
> static int i915_check_nomodeset(void)
> {
> bool use_kms = true;
> + int ret;
>
> /*
>* Enable KMS by default, unless explicitly overriden by
> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
> if (i915_modparams.modeset == 0)
> use_kms = false;
>
> - if (vgacon_text_force() && i915_modparams.modeset == -1)
> + ret = drm_drv_enabled();
You pass the local driver variable here - which looks wrong as this is
not the same as the driver variable declared in another file.
Maybe move the check to new function you can add to init_funcs,
and locate the new function in i915_drv - so it has access to driver?
Sam
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier,
>
> >>>
> >>> - if (vgacon_text_force() && i915_modparams.modeset == -1)
> >>> + ret = drm_drv_enabled();
> >>
> >> You pass the local driver variable here - which looks wrong as this is
> >> not the same as the driver variable declared in another file.
> >
>
> Yes, Jani mentioned it already. I got confused and thought that the driver
> variable was also defined in the same compilation unit...
>
> Maybe I could squash the following change?
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b18a250e5d2e..b8f399b76363 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -89,7 +89,7 @@
> #include "intel_region_ttm.h"
> #include "vlv_suspend.h"
>
> -static const struct drm_driver driver;
> +const struct drm_driver driver;
No, variables with such a generic name will clash.
Just add a
const drm_driver * i915_drm_driver(void)
{
return
}
And then use this function to access the drm_driver variable.
Sam
Re: [Spice-devel] [PATCH] drm/qxl: Convert to Linux IRQ interfaces
On Tue, Jul 06, 2021 at 09:47:35AM +0200, Thomas Zimmermann wrote: > Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's > IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers > don't benefit from using it. > > Signed-off-by: Thomas Zimmermann Looks correct, Acked-by: Sam Ravnborg Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 00/15] drm: Move struct drm_device.pdev to legacy
Hi Thomas. Nice clean-up series - quite an effort to move one member to deprecated! I have read through most of the patches. I left a few notes in my replies but nothing buggy. Just nitpicks. On Tue, Nov 24, 2020 at 12:38:09PM +0100, Thomas Zimmermann wrote: > The pdev field in struct drm_device points to a PCI device structure and > goes back to UMS-only days when all DRM drivers where for PCI devices. > Meanwhile we also support USB, SPI and platform devices. Each of those > uses the generic device stored in struct drm_device.dev. > > To reduce duplications and remove the special case of PCI, this patchset > converts all modesetting drivers from pdev to dev and makes pdev a field > for legacy UMS drivers. > > For PCI devices, the pointer in struct drm_device.dev can be upcasted to > struct pci_device; or tested for PCI with dev_is_pci(). In several places > the code can use the dev field directly. > > After converting all drivers and the DRM core, the pdev fields becomes > only relevant for legacy drivers. In a later patchset, we may want to > convert these as well and remove pdev entirely. > > The patchset touches many files, but the individual changes are mostly > trivial. I suggest to merge each driver's patch through the respective > tree and later the rest through drm-misc-next. > > Thomas Zimmermann (15): > drm/amdgpu: Remove references to struct drm_device.pdev > drm/ast: Remove references to struct drm_device.pdev > drm/bochs: Remove references to struct drm_device.pdev > drm/cirrus: Remove references to struct drm_device.pdev > drm/gma500: Remove references to struct drm_device.pdev > drm/hibmc: Remove references to struct drm_device.pdev > drm/mgag200: Remove references to struct drm_device.pdev > drm/qxl: Remove references to struct drm_device.pdev > drm/vboxvideo: Remove references to struct drm_device.pdev > drm/virtgpu: Remove references to struct drm_device.pdev > drm/vmwgfx: Remove references to struct drm_device.pdev > drm: Upcast struct drm_device.dev to struct pci_device; replace pdev All above are: Acked-by: Sam Ravnborg > drm/nouveau: Remove references to struct drm_device.pdev I lost my confidence in my reading of this code. > drm/i915: Remove references to struct drm_device.pdev > drm/radeon: Remove references to struct drm_device.pdev I did not look at these at all. I hope someone else find time to do so. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 05/15] drm/gma500: Remove references to struct drm_device.pdev
Hi Thomas.
On Tue, Nov 24, 2020 at 12:38:14PM +0100, Thomas Zimmermann wrote:
> Using struct drm_device.pdev is deprecated. Convert gma500 to struct
> drm_device.dev. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Patrik Jakobsson
This patch includes several whitespace changes too.
It would be nice to avoid these as the patch is already large enough.
Browsing the patch it was not so many, it looked like more in the start
of the patch.
Sam
> ---
> drivers/gpu/drm/gma500/cdv_device.c| 30 +++---
> drivers/gpu/drm/gma500/cdv_intel_crt.c | 3 +-
> drivers/gpu/drm/gma500/cdv_intel_lvds.c| 4 +--
> drivers/gpu/drm/gma500/framebuffer.c | 9 +++---
> drivers/gpu/drm/gma500/gma_device.c| 3 +-
> drivers/gpu/drm/gma500/gma_display.c | 4 +--
> drivers/gpu/drm/gma500/gtt.c | 20 ++--
> drivers/gpu/drm/gma500/intel_bios.c| 6 ++--
> drivers/gpu/drm/gma500/intel_gmbus.c | 4 +--
> drivers/gpu/drm/gma500/intel_i2c.c | 2 +-
> drivers/gpu/drm/gma500/mdfld_device.c | 4 ++-
> drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 8 ++---
> drivers/gpu/drm/gma500/mid_bios.c | 9 --
> drivers/gpu/drm/gma500/oaktrail_device.c | 5 +--
> drivers/gpu/drm/gma500/oaktrail_lvds.c | 2 +-
> drivers/gpu/drm/gma500/oaktrail_lvds_i2c.c | 2 +-
> drivers/gpu/drm/gma500/opregion.c | 3 +-
> drivers/gpu/drm/gma500/power.c | 13
> drivers/gpu/drm/gma500/psb_drv.c | 16 +-
> drivers/gpu/drm/gma500/psb_drv.h | 8 ++---
> drivers/gpu/drm/gma500/psb_intel_lvds.c| 6 ++--
> drivers/gpu/drm/gma500/psb_intel_sdvo.c| 2 +-
> drivers/gpu/drm/gma500/tc35876x-dsi-lvds.c | 36 +++---
> 23 files changed, 109 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_device.c
> b/drivers/gpu/drm/gma500/cdv_device.c
> index e75293e4a52f..19e055dbd4c2 100644
> --- a/drivers/gpu/drm/gma500/cdv_device.c
> +++ b/drivers/gpu/drm/gma500/cdv_device.c
> @@ -95,13 +95,14 @@ static u32 cdv_get_max_backlight(struct drm_device *dev)
> static int cdv_get_brightness(struct backlight_device *bd)
> {
> struct drm_device *dev = bl_get_data(bd);
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> u32 val = REG_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>
> if (cdv_backlight_combination_mode(dev)) {
> u8 lbpc;
>
> val &= ~1;
> - pci_read_config_byte(dev->pdev, 0xF4, );
> + pci_read_config_byte(pdev, 0xF4, );
> val *= lbpc;
> }
> return (val * 100)/cdv_get_max_backlight(dev);
> @@ -111,6 +112,7 @@ static int cdv_get_brightness(struct backlight_device *bd)
> static int cdv_set_brightness(struct backlight_device *bd)
> {
> struct drm_device *dev = bl_get_data(bd);
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> int level = bd->props.brightness;
> u32 blc_pwm_ctl;
>
> @@ -128,7 +130,7 @@ static int cdv_set_brightness(struct backlight_device *bd)
> lbpc = level * 0xfe / max + 1;
> level /= lbpc;
>
> - pci_write_config_byte(dev->pdev, 0xF4, lbpc);
> + pci_write_config_byte(pdev, 0xF4, lbpc);
> }
>
> blc_pwm_ctl = REG_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK;
> @@ -205,8 +207,9 @@ static inline void CDV_MSG_WRITE32(int domain, uint port,
> uint offset,
> static void cdv_init_pm(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> u32 pwr_cnt;
> - int domain = pci_domain_nr(dev->pdev->bus);
> + int domain = pci_domain_nr(pdev->bus);
> int i;
>
> dev_priv->apm_base = CDV_MSG_READ32(domain, PSB_PUNIT_PORT,
> @@ -234,6 +237,8 @@ static void cdv_init_pm(struct drm_device *dev)
>
> static void cdv_errata(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> /* Disable bonus launch.
>* CPU and GPU competes for memory and display misses updates and
>* flickers. Worst with dual core, dual displays.
> @@ -242,7 +247,7 @@ static void cdv_errata(struct drm_device *dev)
>* Bonus Launch to work around the issue, by degrading
>* performance.
>*/
> - CDV_MSG_WRITE32(pci_domain_nr(dev->pdev->bus), 3, 0x30, 0x08027108);
> + CDV_MSG_WRITE32(pci_domain_nr(pdev->bus), 3, 0x30, 0x08027108);
> }
>
> /**
> @@ -255,12 +260,13 @@ static void cdv_errata(struct drm_device *dev)
> static int cdv_save_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct psb_save_area *regs = _priv->regs;
> struct drm_connector *connector;
>
> dev_dbg(dev->dev, "Saving GPU registers.\n");
>
Re: [Spice-devel] [PATCH 09/15] drm/nouveau: Remove references to struct drm_device.pdev
Hi Thomas.
On Tue, Nov 24, 2020 at 12:38:18PM +0100, Thomas Zimmermann wrote:
> Using struct drm_device.pdev is deprecated. Convert nouveau to struct
> drm_device.dev. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Ben Skeggs
Suggestion to an alternative implmentation below.
> ---
> drivers/gpu/drm/nouveau/dispnv04/arb.c | 12 +++-
> drivers/gpu/drm/nouveau/dispnv04/disp.h | 14 --
> drivers/gpu/drm/nouveau/dispnv04/hw.c | 10 ++
> drivers/gpu/drm/nouveau/nouveau_abi16.c | 7 ---
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_bios.c | 11 ---
> drivers/gpu/drm/nouveau/nouveau_connector.c | 10 ++
> drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++---
> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 6 --
> drivers/gpu/drm/nouveau/nouveau_vga.c | 20
> 10 files changed, 58 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c
> b/drivers/gpu/drm/nouveau/nouveau_bios.c
> index d204ea8a5618..7cc683b8dc7a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bios.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
> @@ -110,6 +110,9 @@ static int call_lvds_manufacturer_script(struct
> drm_device *dev, struct dcb_outp
> struct nvbios *bios = >vbios;
> uint8_t sub = bios->data[bios->fp.xlated_entry + script] +
> (bios->fp.link_c_increment && dcbent->or & DCB_OUTPUT_C ? 1 : 0);
> uint16_t scriptofs = ROM16(bios->data[bios->init_script_tbls_ptr + sub
> * 2]);
> +#ifdef __powerpc__
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +#endif
Or
int device = 0;
>
> if (!bios->fp.xlated_entry || !sub || !scriptofs)
> return -EINVAL;
> @@ -123,8 +126,8 @@ static int call_lvds_manufacturer_script(struct
> drm_device *dev, struct dcb_outp
> #ifdef __powerpc__
> /* Powerbook specific quirks */
device = to_pci_dev(dev->dev)->device;
if (script == LVDS_RESET && (device == 0x0179 || device == 0x0189 ||
device == 0x0329))
> if (script == LVDS_RESET &&
> - (dev->pdev->device == 0x0179 || dev->pdev->device == 0x0189 ||
> - dev->pdev->device == 0x0329))
> + (pdev->device == 0x0179 || pdev->device == 0x0189 ||
> + pdev->device == 0x0329))
> nv_write_tmds(dev, dcbent->or, 0, 0x02, 0x72);
> #endif
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index 24ec5339efb4..4fc0fa696461 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -396,7 +396,9 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
> NV_INFO(drm, "allocated %dx%d fb: 0x%llx, bo %p\n",
> fb->width, fb->height, nvbo->offset, nvbo);
>
> - vga_switcheroo_client_fb_set(dev->pdev, info);
> + if (dev_is_pci(dev->dev))
> + vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), info);
> +
I cannot see why dev_is_pci() is needed here.
So I am obviously missing something :-(
> return 0;
>
> out_unlock:
> @@ -548,7 +550,7 @@ nouveau_fbcon_init(struct drm_device *dev)
> int ret;
>
> if (!dev->mode_config.num_crtc ||
> - (dev->pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + (to_pci_dev(dev->dev)->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> return 0;
>
> fbcon = kzalloc(sizeof(struct nouveau_fbdev), GFP_KERNEL);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
> b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index c85dd8afa3c3..7c4b374b3eca 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -87,18 +87,20 @@ nouveau_vga_init(struct nouveau_drm *drm)
> {
> struct drm_device *dev = drm->dev;
> bool runtime = nouveau_pmops_runtime();
> + struct pci_dev *pdev;
>
> /* only relevant for PCI devices */
> - if (!dev->pdev)
> + if (!dev_is_pci(dev->dev))
> return;
> + pdev = to_pci_dev(dev->dev);
>
> - vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
> + vga_client_register(pdev, dev, NULL, nouveau_vga_set_decode);
>
> /* don't register Thunderbolt eGPU with vga_switcheroo */
> - if (pci_is_thunderbolt_attached(dev->pdev))
> + if (pci_is_thunderbolt_attached(pdev))
> return;
>
> - vga_switcheroo_register_client(dev->pdev, _switcheroo_ops,
> runtime);
> + vga_switcheroo_register_client(pdev, _switcheroo_ops, runtime);
>
> if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> vga_switcheroo_init_domain_pm_ops(drm->dev->dev,
> >vga_pm_domain);
> @@ -109,17 +111,19 @@ nouveau_vga_fini(struct nouveau_drm *drm)
> {
> struct drm_device *dev = drm->dev;
> bool runtime = nouveau_pmops_runtime();
> + struct pci_dev *pdev;
>
> /* only
Re: [Spice-devel] [PATCH 15/15] drm: Upcast struct drm_device.dev to struct pci_device; replace pdev
Hi Thomas,
On Tue, Nov 24, 2020 at 12:38:24PM +0100, Thomas Zimmermann wrote:
> We have DRM drivers based on USB, SPI and platform devices. All of them
> are fine with storing their device reference in struct drm_device.dev.
> PCI devices should be no exception. Therefore struct drm_device.pdev is
> deprecated.
>
> Instead upcast from struct drm_device.dev with to_pci_dev(). PCI-specific
> code can use dev_is_pci() to test for a PCI device. This patch changes
> the DRM core code and documentation accordingly. Struct drm_device.pdev
> is being moved to legacy status.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_agpsupport.c | 9 ++---
> drivers/gpu/drm/drm_bufs.c | 4 ++--
> drivers/gpu/drm/drm_edid.c | 7 ++-
> drivers/gpu/drm/drm_irq.c| 12 +++-
> drivers/gpu/drm/drm_pci.c| 26 +++---
> drivers/gpu/drm/drm_vm.c | 2 +-
> include/drm/drm_device.h | 12 +---
> 7 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_agpsupport.c
> b/drivers/gpu/drm/drm_agpsupport.c
> index 4c7ad46fdd21..a4040fe4f4ba 100644
> --- a/drivers/gpu/drm/drm_agpsupport.c
> +++ b/drivers/gpu/drm/drm_agpsupport.c
> @@ -103,11 +103,13 @@ int drm_agp_info_ioctl(struct drm_device *dev, void
> *data,
> */
> int drm_agp_acquire(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> if (!dev->agp)
> return -ENODEV;
> if (dev->agp->acquired)
> return -EBUSY;
> - dev->agp->bridge = agp_backend_acquire(dev->pdev);
> + dev->agp->bridge = agp_backend_acquire(pdev);
> if (!dev->agp->bridge)
> return -ENODEV;
> dev->agp->acquired = 1;
> @@ -402,14 +404,15 @@ int drm_agp_free_ioctl(struct drm_device *dev, void
> *data,
> */
> struct drm_agp_head *drm_agp_init(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct drm_agp_head *head = NULL;
>
> head = kzalloc(sizeof(*head), GFP_KERNEL);
> if (!head)
> return NULL;
> - head->bridge = agp_find_bridge(dev->pdev);
> + head->bridge = agp_find_bridge(pdev);
> if (!head->bridge) {
> - head->bridge = agp_backend_acquire(dev->pdev);
> + head->bridge = agp_backend_acquire(pdev);
> if (!head->bridge) {
> kfree(head);
> return NULL;
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index 7a01d0918861..1da8b360b60a 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -325,7 +325,7 @@ static int drm_addmap_core(struct drm_device *dev,
> resource_size_t offset,
>* As we're limiting the address to 2^32-1 (or less),
>* casting it down to 32 bits is no problem, but we
>* need to point to a 64bit variable first. */
> - map->handle = dma_alloc_coherent(>pdev->dev,
> + map->handle = dma_alloc_coherent(dev->dev,
>map->size,
>>offset,
>GFP_KERNEL);
> @@ -555,7 +555,7 @@ int drm_legacy_rmmap_locked(struct drm_device *dev,
> struct drm_local_map *map)
> case _DRM_SCATTER_GATHER:
> break;
> case _DRM_CONSISTENT:
> - dma_free_coherent(>pdev->dev,
> + dma_free_coherent(dev->dev,
> map->size,
> map->handle,
> map->offset);
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 74f5a3197214..555a04ce2179 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -2075,9 +2076,13 @@ EXPORT_SYMBOL(drm_get_edid);
> struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>struct i2c_adapter *adapter)
> {
> - struct pci_dev *pdev = connector->dev->pdev;
> + struct drm_device *dev = connector->dev;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct edid *edid;
Maybe add a comment that explain why this can trigger - so people are
helped it they are catched by this.
As it is now it is not even mentioned in the changelog.
> + if (drm_WARN_ON_ONCE(dev, !dev_is_pci(dev->dev)))
> + return NULL;
> +
> vga_switcheroo_lock_ddc(pdev);
> edid = drm_get_edid(connector, adapter);
> vga_switcheroo_unlock_ddc(pdev);
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 09d6e9e2e075..22986a9a593b 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -122,7 +122,7 @@ int drm_irq_install(struct drm_device *dev, int
Re: [Spice-devel] [PATCH] drm: remove unneeded break
Hi Tom On Mon, Oct 19, 2020 at 07:06:41PM +0200, Sam Ravnborg wrote: > Hi Tom > On Mon, Oct 19, 2020 at 09:31:15AM -0700, t...@redhat.com wrote: > > From: Tom Rix > > > > A break is not needed if it is preceded by a return or break > > > > Signed-off-by: Tom Rix > > Looks good and builds with no warnings. > > One of the diffs made me - "oh this looks wrong". But after I looked again > it was right and the resulting code is more readable - so good. > > Acked-by: Sam Ravnborg > > Was tempted to just apply to drm-misc-next but will give others the > opportunity to chime in. Thanks. Now applied to drm-misc-next - will show up in -next in a week or so. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v5 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions. Read and write operations are implemented
> internally by DRM's fbdev helper.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v5:
> * implement fb_read/fb_write internally (Daniel, Sam)
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
>
> Signed-off-by: Thomas Zimmermann
> Tested-by: Sam Ravnborg
Reviewed-by: Sam Ravnborg
But see a few comments below on naming for you to consider.
Sam
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 227 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 230 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..1d3180841778 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
Re: [Spice-devel] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
On Fri, Oct 16, 2020 at 02:19:42PM +0200, Thomas Zimmermann wrote: > Hi > > On Fri, 16 Oct 2020 14:03:47 +0200 Sam Ravnborg wrote: > > > Hi Thomas. > > > > On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > > > At least sparc64 requires I/O-specific access to framebuffers. This > > > patch updates the fbdev console accordingly. > > > > > > For drivers with direct access to the framebuffer memory, the callback > > > functions in struct fb_ops test for the type of memory and call the rsp > > > fb_sys_ of fb_cfb_ functions. > > > > > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > > > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > > > interfaces to access the buffer. > > > > > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > > > I/O memory and avoid a HW exception. With the introduction of struct > > > dma_buf_map, this is not required any longer. The patch removes the rsp > > > code from both, bochs and fbdev. > > > > > > v4: > > > * move dma_buf_map changes into separate patch (Daniel) > > > * TODO list: comment on fbdev updates (Daniel) > > > > > > Signed-off-by: Thomas Zimmermann > > > > The original workaround fixed it so we could run qemu with the > > -nographic option. > > > > So I went ahead and tried to run quemu version: > > v5.0.0-1970-g0b100c8e72-dirty. > > And with the BOCHS driver built-in. > > > > With the following command line: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic > > > > Behaviour was the same before and after applying this patch. > > (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) > > So I consider it fixed for real now and not just a workaround. > > > > I also tested with: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial > > stdio > > > > and it worked in both cases too. > > FTR, you booted a kernel and got graphics output. The error is simply that > there was no disk to mount? The short version "Yes". The longer version: With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio" I got graphical output - one penguin. With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic" I got no graphical output, as implied by the -nographic option. But the boot continued - where it would panic before when we accessed IO memory as system memory. In both cases I got an error because I had not specified any rootfs, so qemu failed to mount any rootfs. So expected. Sam > > Best regards > Thomas > > > > > All the comments above so future-me have an easier time finding how to > > reproduce. > > > > Tested-by: Sam Ravnborg > > > > Sam > > > > > --- > > > Documentation/gpu/todo.rst| 19 ++- > > > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > > > drivers/gpu/drm/drm_fb_helper.c | 217 -- > > > include/drm/drm_mode_config.h | 12 -- > > > 4 files changed, 220 insertions(+), 29 deletions(-) > > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > > index 7e6fc3c04add..638b7f704339 100644 > > > --- a/Documentation/gpu/todo.rst > > > +++ b/Documentation/gpu/todo.rst > > > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > > > > > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > > > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > > > -expects the framebuffer in system memory (or system-like memory). > > > +atomic modesetting and GEM vmap support. Historically, generic fbdev > > > emulation +expected the framebuffer in system memory or system-like > > > memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O > > > memory can be supported +as well. > > > > > > Contact: Maintainer of the driver you plan to convert > > > > > > Level: Intermediate > > > > > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > > > +--- > > > + > > > +A number of callback functions in drm_fbdev_fb_ops could benefit from > > > +being rewritten without depen
Re: [Spice-devel] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas. On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > At least sparc64 requires I/O-specific access to framebuffers. This > patch updates the fbdev console accordingly. > > For drivers with direct access to the framebuffer memory, the callback > functions in struct fb_ops test for the type of memory and call the rsp > fb_sys_ of fb_cfb_ functions. > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > interfaces to access the buffer. > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > I/O memory and avoid a HW exception. With the introduction of struct > dma_buf_map, this is not required any longer. The patch removes the rsp > code from both, bochs and fbdev. > > v4: > * move dma_buf_map changes into separate patch (Daniel) > * TODO list: comment on fbdev updates (Daniel) > > Signed-off-by: Thomas Zimmermann The original workaround fixed it so we could run qemu with the -nographic option. So I went ahead and tried to run quemu version: v5.0.0-1970-g0b100c8e72-dirty. And with the BOCHS driver built-in. With the following command line: qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic Behaviour was the same before and after applying this patch. (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) So I consider it fixed for real now and not just a workaround. I also tested with: qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio and it worked in both cases too. All the comments above so future-me have an easier time finding how to reproduce. Tested-by: Sam Ravnborg Sam > --- > Documentation/gpu/todo.rst| 19 ++- > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > drivers/gpu/drm/drm_fb_helper.c | 217 -- > include/drm/drm_mode_config.h | 12 -- > 4 files changed, 220 insertions(+), 29 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7e6fc3c04add..638b7f704339 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > -expects the framebuffer in system memory (or system-like memory). > +atomic modesetting and GEM vmap support. Historically, generic fbdev > emulation > +expected the framebuffer in system memory or system-like memory. By employing > +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported > +as well. > > Contact: Maintainer of the driver you plan to convert > > Level: Intermediate > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > +--- > + > +A number of callback functions in drm_fbdev_fb_ops could benefit from > +being rewritten without dependencies on the fbdev module. Some of the > +helpers could further benefit from using struct dma_buf_map instead of > +raw pointers. > + > +Contact: Thomas Zimmermann , Daniel Vetter > + > +Level: Advanced > + > + > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > - > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c > b/drivers/gpu/drm/bochs/bochs_kms.c > index 13d0d04c4457..853081d186d5 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > bochs->dev->mode_config.preferred_depth = 24; > bochs->dev->mode_config.prefer_shadow = 0; > bochs->dev->mode_config.prefer_shadow_fbdev = 1; > - bochs->dev->mode_config.fbdev_use_iomem = true; > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; > > bochs->dev->mode_config.funcs = _mode_funcs; > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6212cd7cde1d..462b0c130ebb 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct > work_struct *work) > } > > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > - struct drm_clip_rect *clip) > + struct drm_clip_rect *clip, > + struct dma_bu
Re: [Spice-devel] [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
sparc64 needs "#include " to build as is does not get
this via io.h
Sam
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
I have been offline for a while so have not followed all the threads on
this. So may comments below may well be addressed but I failed to see
it.
If the point about fb_sync is already addressed/considered then:
Reviewed-by: Sam Ravnborg
> Signed-off-by: Thomas Zimmermann
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 217 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 220 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
Good to see this workaround gone again!
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..462b0c130ebb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
> - fo
Re: [Spice-devel] [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
Looks good.
Reviewed-by: Sam Ravnborg
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: Fix build errors
Hi Sean.
On Mon, Aug 17, 2020 at 03:58:38PM -0400, Sean Paul wrote:
> From: Sean Paul
>
> Introduced in the patch below, the END macro was missing 'dev' and BEGIN
> macro needs drm_drv_uses_atomic_modeset() from drm_drv.h
>
> Fixes: bbaac1354cc9 ("drm/qxl: Replace deprecated function in qxl_display")
We should not use Fixes for local fixes like this, as we do not want the
robots to pick this commit.
With the Fixes: dropped (maybe just reference the commit in the
changelog):
Acked-by: Sam Ravnborg
> Cc: Sidong Yang
> Cc: Gerd Hoffmann
> Cc: Dave Airlie
> Cc: virtualizat...@lists.linux-foundation.org
> Signed-off-by: Sean Paul
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index fa79688013b7..5b4fd6952b53 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -28,6 +28,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -186,7 +187,7 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev)
>
> DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE,
> ret);
> qxl_update_offset_props(qdev);
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> if (!drm_helper_hpd_irq_event(dev)) {
> /* notify that the monitor configuration changed, to
> adjust at the arbitrary resolution */
> @@ -431,7 +432,7 @@ static int qxl_framebuffer_surface_dirty(struct
> drm_framebuffer *fb,
> clips, num_clips, inc, 0);
>
> out_lock_end:
> - DRM_MODESET_LOCK_ALL_END(ctx, ret);
> + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
>
> return 0;
> }
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 26/59] drm/qxl: Use devm_drm_dev_alloc
On Tue, Apr 28, 2020 at 04:00:11PM +0200, Daniel Vetter wrote:
> On Fri, Apr 24, 2020 at 05:09:11PM +0200, Sam Ravnborg wrote:
> > Hi Daniel
> >
> > On Wed, Apr 15, 2020 at 09:40:01AM +0200, Daniel Vetter wrote:
> > > Also need to remove the drm_dev_put from the remove hook.
> > >
> > > Acked-by: Gerd Hoffmann
> > > Signed-off-by: Daniel Vetter
> > > Cc: Dave Airlie
> > > Cc: Gerd Hoffmann
> > > Cc: virtualizat...@lists.linux-foundation.org
> > > Cc: spice-devel@lists.freedesktop.org
> > > ---
> > > drivers/gpu/drm/qxl/qxl_drv.c | 15 ---
> > > drivers/gpu/drm/qxl/qxl_drv.h | 3 +--
> > > drivers/gpu/drm/qxl/qxl_kms.c | 12 +---
> > > 3 files changed, 10 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > > index 09102e2efabc..6b4ae4c5fb76 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > > @@ -81,13 +81,16 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> > > pci_device_id *ent)
> > > return -EINVAL; /* TODO: ENODEV ? */
> > > }
> > >
> > > - qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
> > > - if (!qdev)
> > > + qdev = devm_drm_dev_alloc(>dev, _driver,
> > > + struct qxl_device, ddev);
> > > + if (IS_ERR(qdev)) {
> > > + pr_err("Unable to init drm dev");
> > > return -ENOMEM;
> > > + }
> >
> > The other patches do not add any error message when devm_drm_dev_alloc()
> > fails and driver core will log that driver init failed.
> >
> > So the pr_err() above should be dropped.
> > I know it comes from qxl_device_init() but that does not make it a good
> > idea.
>
> Hm I know we're inconsistent here, but some drivers have error logging on
> all branches, some dont. I'm just trying to go with the prevailing style.
>
> > With this fixed:
>
> Insisting on this or ok as-is?
OK as-is.
Sam
> -Daniel
>
> > Acked-by: Sam Ravnborg
> >
> > >
> > > ret = pci_enable_device(pdev);
> > > if (ret)
> > > - goto free_dev;
> > > + return ret;
> > >
> > > ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "qxl");
> > > if (ret)
> > > @@ -101,7 +104,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> > > pci_device_id *ent)
> > > }
> > > }
> > >
> > > - ret = qxl_device_init(qdev, _driver, pdev);
> > > + ret = qxl_device_init(qdev, pdev);
> > > if (ret)
> > > goto put_vga;
> > >
> > > @@ -128,8 +131,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> > > pci_device_id *ent)
> > > vga_put(pdev, VGA_RSRC_LEGACY_IO);
> > > disable_pci:
> > > pci_disable_device(pdev);
> > > -free_dev:
> > > - kfree(qdev);
> > > +
> > > return ret;
> > > }
> > >
> > > @@ -155,7 +157,6 @@ qxl_pci_remove(struct pci_dev *pdev)
> > > drm_atomic_helper_shutdown(dev);
> > > if (is_vga(pdev))
> > > vga_put(pdev, VGA_RSRC_LEGACY_IO);
> > > - drm_dev_put(dev);
> > > }
> > >
> > > DEFINE_DRM_GEM_FOPS(qxl_fops);
> > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> > > index 435126facc9b..86ac191d9205 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_drv.h
> > > +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> > > @@ -276,8 +276,7 @@ struct qxl_device {
> > > extern const struct drm_ioctl_desc qxl_ioctls[];
> > > extern int qxl_max_ioctl;
> > >
> > > -int qxl_device_init(struct qxl_device *qdev, struct drm_driver *drv,
> > > - struct pci_dev *pdev);
> > > +int qxl_device_init(struct qxl_device *qdev, struct pci_dev *pdev);
> > > void qxl_device_fini(struct qxl_device *qdev);
> > >
> > > int qxl_modeset_init(struct qxl_device *qdev);
> > > diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> > > index 9eed1a375f24..91a34dd835d7 100644
> > > --- a/drivers/gpu/drm/qxl/qxl_kms.c
> > > +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> > > @@ -108,21 +108,13 @@ static void qxl_gc_work(struct work_struct *work)
> > > }
> > >
> > > int qxl_device_init(struct
Re: [Spice-devel] [PATCH 27/59] drm/qxl: Don't use drm_device->dev_private
On Wed, Apr 15, 2020 at 09:40:02AM +0200, Daniel Vetter wrote:
> Upcasting using a container_of macro is more typesafe, faster and
> easier for the compiler to optimize.
>
> Acked-by: Gerd Hoffmann
> Signed-off-by: Daniel Vetter
> Cc: Dave Airlie
> Cc: Gerd Hoffmann
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: spice-devel@lists.freedesktop.org
Acked-by: Sam Ravnborg
> ---
> drivers/gpu/drm/qxl/qxl_debugfs.c | 7 +++
> drivers/gpu/drm/qxl/qxl_display.c | 32 +++
> drivers/gpu/drm/qxl/qxl_drv.c | 8
> drivers/gpu/drm/qxl/qxl_drv.h | 4 ++--
> drivers/gpu/drm/qxl/qxl_dumb.c| 2 +-
> drivers/gpu/drm/qxl/qxl_gem.c | 2 +-
> drivers/gpu/drm/qxl/qxl_ioctl.c | 14 +++---
> drivers/gpu/drm/qxl/qxl_irq.c | 2 +-
> drivers/gpu/drm/qxl/qxl_kms.c | 1 -
> drivers/gpu/drm/qxl/qxl_object.c | 2 +-
> drivers/gpu/drm/qxl/qxl_release.c | 2 +-
> drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
> 12 files changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_debugfs.c
> b/drivers/gpu/drm/qxl/qxl_debugfs.c
> index 88123047fdd4..524d35b648d8 100644
> --- a/drivers/gpu/drm/qxl/qxl_debugfs.c
> +++ b/drivers/gpu/drm/qxl/qxl_debugfs.c
> @@ -39,7 +39,7 @@ static int
> qxl_debugfs_irq_received(struct seq_file *m, void *data)
> {
> struct drm_info_node *node = (struct drm_info_node *) m->private;
> - struct qxl_device *qdev = node->minor->dev->dev_private;
> + struct qxl_device *qdev = to_qxl(node->minor->dev);
>
> seq_printf(m, "%d\n", atomic_read(>irq_received));
> seq_printf(m, "%d\n", atomic_read(>irq_received_display));
> @@ -53,7 +53,7 @@ static int
> qxl_debugfs_buffers_info(struct seq_file *m, void *data)
> {
> struct drm_info_node *node = (struct drm_info_node *) m->private;
> - struct qxl_device *qdev = node->minor->dev->dev_private;
> + struct qxl_device *qdev = to_qxl(node->minor->dev);
> struct qxl_bo *bo;
>
> list_for_each_entry(bo, >gem.objects, list) {
> @@ -83,8 +83,7 @@ void
> qxl_debugfs_init(struct drm_minor *minor)
> {
> #if defined(CONFIG_DEBUG_FS)
> - struct qxl_device *dev =
> - (struct qxl_device *) minor->dev->dev_private;
> + struct qxl_device *dev = to_qxl(minor->dev);
>
> drm_debugfs_create_files(qxl_debugfs_list, QXL_DEBUGFS_ENTRIES,
>minor->debugfs_root, minor);
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 09583a08e141..1082cd5d2fd4 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -221,7 +221,7 @@ static int qxl_add_mode(struct drm_connector *connector,
> bool preferred)
> {
> struct drm_device *dev = connector->dev;
> - struct qxl_device *qdev = dev->dev_private;
> + struct qxl_device *qdev = to_qxl(dev);
> struct drm_display_mode *mode = NULL;
> int rc;
>
> @@ -242,7 +242,7 @@ static int qxl_add_mode(struct drm_connector *connector,
> static int qxl_add_monitors_config_modes(struct drm_connector *connector)
> {
> struct drm_device *dev = connector->dev;
> - struct qxl_device *qdev = dev->dev_private;
> + struct qxl_device *qdev = to_qxl(dev);
> struct qxl_output *output = drm_connector_to_qxl_output(connector);
> int h = output->index;
> struct qxl_head *head;
> @@ -310,7 +310,7 @@ static void qxl_crtc_update_monitors_config(struct
> drm_crtc *crtc,
> const char *reason)
> {
> struct drm_device *dev = crtc->dev;
> - struct qxl_device *qdev = dev->dev_private;
> + struct qxl_device *qdev = to_qxl(dev);
> struct qxl_crtc *qcrtc = to_qxl_crtc(crtc);
> struct qxl_head head;
> int oldcount, i = qcrtc->index;
> @@ -400,7 +400,7 @@ static int qxl_framebuffer_surface_dirty(struct
> drm_framebuffer *fb,
>unsigned int num_clips)
> {
> /* TODO: vmwgfx where this was cribbed from had locking. Why? */
> - struct qxl_device *qdev = fb->dev->dev_private;
> + struct qxl_device *qdev = to_qxl(fb->dev);
> struct drm_clip_rect norect;
> struct qxl_bo *qobj;
> bool is_primary;
> @@ -462,7 +462,7 @@ static const struct drm_crtc_helper_funcs
> qxl_crtc_helper_funcs = {
> static int qxl_primary_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> - struct qxl_dev
Re: [Spice-devel] [PATCH 26/59] drm/qxl: Use devm_drm_dev_alloc
Hi Daniel
On Wed, Apr 15, 2020 at 09:40:01AM +0200, Daniel Vetter wrote:
> Also need to remove the drm_dev_put from the remove hook.
>
> Acked-by: Gerd Hoffmann
> Signed-off-by: Daniel Vetter
> Cc: Dave Airlie
> Cc: Gerd Hoffmann
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: spice-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/qxl/qxl_drv.c | 15 ---
> drivers/gpu/drm/qxl/qxl_drv.h | 3 +--
> drivers/gpu/drm/qxl/qxl_kms.c | 12 +---
> 3 files changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 09102e2efabc..6b4ae4c5fb76 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -81,13 +81,16 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> return -EINVAL; /* TODO: ENODEV ? */
> }
>
> - qdev = kzalloc(sizeof(struct qxl_device), GFP_KERNEL);
> - if (!qdev)
> + qdev = devm_drm_dev_alloc(>dev, _driver,
> + struct qxl_device, ddev);
> + if (IS_ERR(qdev)) {
> + pr_err("Unable to init drm dev");
> return -ENOMEM;
> + }
The other patches do not add any error message when devm_drm_dev_alloc()
fails and driver core will log that driver init failed.
So the pr_err() above should be dropped.
I know it comes from qxl_device_init() but that does not make it a good
idea.
With this fixed:
Acked-by: Sam Ravnborg
>
> ret = pci_enable_device(pdev);
> if (ret)
> - goto free_dev;
> + return ret;
>
> ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "qxl");
> if (ret)
> @@ -101,7 +104,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> }
> }
>
> - ret = qxl_device_init(qdev, _driver, pdev);
> + ret = qxl_device_init(qdev, pdev);
> if (ret)
> goto put_vga;
>
> @@ -128,8 +131,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> vga_put(pdev, VGA_RSRC_LEGACY_IO);
> disable_pci:
> pci_disable_device(pdev);
> -free_dev:
> - kfree(qdev);
> +
> return ret;
> }
>
> @@ -155,7 +157,6 @@ qxl_pci_remove(struct pci_dev *pdev)
> drm_atomic_helper_shutdown(dev);
> if (is_vga(pdev))
> vga_put(pdev, VGA_RSRC_LEGACY_IO);
> - drm_dev_put(dev);
> }
>
> DEFINE_DRM_GEM_FOPS(qxl_fops);
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 435126facc9b..86ac191d9205 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -276,8 +276,7 @@ struct qxl_device {
> extern const struct drm_ioctl_desc qxl_ioctls[];
> extern int qxl_max_ioctl;
>
> -int qxl_device_init(struct qxl_device *qdev, struct drm_driver *drv,
> - struct pci_dev *pdev);
> +int qxl_device_init(struct qxl_device *qdev, struct pci_dev *pdev);
> void qxl_device_fini(struct qxl_device *qdev);
>
> int qxl_modeset_init(struct qxl_device *qdev);
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index 9eed1a375f24..91a34dd835d7 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -108,21 +108,13 @@ static void qxl_gc_work(struct work_struct *work)
> }
>
> int qxl_device_init(struct qxl_device *qdev,
> - struct drm_driver *drv,
> struct pci_dev *pdev)
> {
> int r, sb;
>
> - r = drm_dev_init(>ddev, drv, >dev);
> - if (r) {
> - pr_err("Unable to init drm dev");
> - goto error;
> - }
> -
> qdev->ddev.pdev = pdev;
> pci_set_drvdata(pdev, >ddev);
> qdev->ddev.dev_private = qdev;
> - drmm_add_final_kfree(>ddev, qdev);
>
> mutex_init(>gem.mutex);
> mutex_init(>update_area_mutex);
> @@ -138,8 +130,7 @@ int qxl_device_init(struct qxl_device *qdev,
> qdev->vram_mapping = io_mapping_create_wc(qdev->vram_base,
> pci_resource_len(pdev, 0));
> if (!qdev->vram_mapping) {
> pr_err("Unable to create vram_mapping");
> - r = -ENOMEM;
> - goto error;
> + return -ENOMEM;
> }
>
> if (pci_resource_len(pdev, 4) > 0) {
> @@ -293,7 +284,6 @@ int qxl_device_init(struct qxl_device *qdev,
> io_mapping_free(qdev->surface_mapping);
> vram_mapping_free:
> io_mapping_free(qdev->vram_mapping);
> -error:
> return r;
> }
>
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v4 0/4] drm: Provide a simple encoder
Hi Thomas. On Fri, Feb 28, 2020 at 09:18:24AM +0100, Thomas Zimmermann wrote: > Many DRM drivers implement an encoder with an empty implementation. This > patchset adds drm_simple_encoder_init(), which drivers can use instead. > Except for the destroy callback, the simple encoder's implementation is > empty. > > The patchset also converts 4 encoder instances to use the simple-encoder > helpers. But there are at least 11 other drivers which can use the helper > and I think I did not examine all drivers yet. > > The patchset was smoke-tested on mgag200 by running the fbdev console > and Gnome on X11. > > v4: > * print error messages with drm_err() (Sam) > * qxl: handle errors of drm_simple_encoder_init() (Sam) Looked through the patches - all looked good. IMO ready to apply. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 3/4] drm/mgag200: Use simple encoder
m_encoder *encoder = >encoder;
> struct drm_connector *connector;
> + int ret;
>
> mdev->mode_info.mode_config_initialized = true;
>
> @@ -1698,11 +1630,14 @@ int mgag200_modeset_init(struct mga_device *mdev)
>
> mga_crtc_init(mdev);
>
> - encoder = mga_encoder_init(mdev->dev);
> - if (!encoder) {
> - DRM_ERROR("mga_encoder_init failed\n");
> - return -1;
> + ret = drm_simple_encoder_init(mdev->dev, encoder,
> + DRM_MODE_ENCODER_DAC);
> + if (ret) {
> + DRM_ERROR("drm_simple_encoder_init() failed, error %d\n",
> + -ret);
DRM_ERROR is deprecated if you have a drm_device.
Consider to use:
drm_err(mdev->dev, "drm_simple_encoder_init() failed, error
%d\n",
ret);
Note - "-ret" looked strange. We usually do not modify return values
like this when printing.
> + return ret;
> }
> + encoder->possible_crtcs = 0x1;
>
> connector = mga_vga_init(mdev->dev);
> if (!connector) {
With the above addressed:
Reviewed-by: Sam Ravnborg
Sam
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 4/4] drm/qxl: Use simple encoder
Hi Thomas.
On Tue, Feb 25, 2020 at 02:10:55PM +0100, Thomas Zimmermann wrote:
> The qxl driver uses an empty implementation for its encoder. Replace
> the code with the generic simple encoder.
>
> v2:
> * rebase onto new simple-encoder interface
>
> Signed-off-by: Thomas Zimmermann
> Acked-by: Sam Ravnborg
> Acked-by: Gerd Hoffmann
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 18 +++---
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ab4f8dd00400..9c0e1add59fb 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "qxl_drv.h"
> #include "qxl_object.h"
> @@ -1007,9 +1008,6 @@ static struct drm_encoder *qxl_best_encoder(struct
> drm_connector *connector)
> return _output->enc;
> }
>
> -static const struct drm_encoder_helper_funcs qxl_enc_helper_funcs = {
> -};
> -
> static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = {
> .get_modes = qxl_conn_get_modes,
> .mode_valid = qxl_conn_mode_valid,
> @@ -1059,15 +1057,6 @@ static const struct drm_connector_funcs
> qxl_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static void qxl_enc_destroy(struct drm_encoder *encoder)
> -{
> - drm_encoder_cleanup(encoder);
> -}
> -
> -static const struct drm_encoder_funcs qxl_enc_funcs = {
> - .destroy = qxl_enc_destroy,
> -};
> -
> static int qxl_mode_create_hotplug_mode_update_property(struct qxl_device
> *qdev)
> {
> if (qdev->hotplug_mode_update_property)
> @@ -1098,15 +1087,14 @@ static int qdev_output_init(struct drm_device *dev,
> int num_output)
> drm_connector_init(dev, _output->base,
> _connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
>
> - drm_encoder_init(dev, _output->enc, _enc_funcs,
> - DRM_MODE_ENCODER_VIRTUAL, NULL);
> + drm_simple_encoder_init(dev, _output->enc,
> + DRM_MODE_ENCODER_VIRTUAL);
return value is ignored - as it was before.
A quick grep told that it is 50/50 if return value is checked.
So OK.
Reviewed-by: Sam Ravnborg
>
> /* we get HPD via client monitors config */
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> encoder->possible_crtcs = 1 << num_output;
> drm_connector_attach_encoder(_output->base,
> _output->enc);
> - drm_encoder_helper_add(encoder, _enc_helper_funcs);
> drm_connector_helper_add(connector, _connector_helper_funcs);
>
> drm_object_attach_property(>base,
> --
> 2.25.0
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
On Tue, Feb 25, 2020 at 02:10:52PM +0100, Thomas Zimmermann wrote:
> This patch makes the internal encoder implementation of the simple
> KMS helpers available to drivers.
>
> These simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
>
> v3:
> * remove drm_simple_encoder_create(); not required yet
> * provide more precise documentation
> v2:
> * move simple encoder to KMS helpers
> * remove name argument; simplifies implementation
> * don't allocate with devm_ interfaces; unsafe with DRM
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Sam Ravnborg
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 34 ++---
> include/drm/drm_simple_kms_helper.h | 4 +++
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..04309e4660de 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,41 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated _connector object and supporting optional _bridge
> * encoder drivers.
> + *
> + * Many drivers require only a very simple encoder that fulfills the minimum
> + * requirements of the display pipeline and does not add additional
> + * functionality. The function drm_simple_encoder_init() provides an
> + * implementation of such an encoder.
> */
>
> -static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> .destroy = drm_encoder_cleanup,
> };
>
> +/**
> + * drm_simple_encoder_init - Initialize a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality.
> + * Settings for possible CRTC and clones are left to their initial values.
> + * The encoder will be cleaned up automatically as part of the mode-setting
> + * cleanup.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type)
> +{
> + return drm_encoder_init(dev, encoder,
> + _simple_encoder_funcs_cleanup,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +317,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> return ret;
>
> encoder->possible_crtcs = drm_crtc_mask(crtc);
> - ret = drm_encoder_init(dev, encoder, _simple_kms_encoder_funcs,
> -DRM_MODE_ENCODER_NONE, NULL);
> + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE);
> if (ret || !connector)
> return ret;
>
> diff --git a/include/drm/drm_simple_kms_helper.h
> b/include/drm/drm_simple_kms_helper.h
> index e253ba7bea9d..a026375464ff 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -181,4 +181,8 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> const uint64_t *format_modifiers,
> struct drm_connector *connector);
>
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type);
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.25.0
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 3/4] drm/mgag200: Use simple encoder
Hi Thomas.
On Fri, Feb 21, 2020 at 08:48:48AM +0100, Thomas Zimmermann wrote:
> Hi Sam
>
> thanks for reviewing the patch set.
>
> Am 20.02.20 um 19:56 schrieb Sam Ravnborg:
> > Hi Thomas.
> >
> > On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
> >> The mgag200 driver uses an empty implementation for its encoder. Replace
> >> the code with the generic simple encoder.
> >>
> >> v2:
> >>* rebase onto new simple-encoder interface
> >>
> >> Signed-off-by: Thomas Zimmermann
> >> ---
> >> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
> >> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++
> >> 2 files changed, 3 insertions(+), 65 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> b/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> index aa32aad222c2..9bb9e8e14539 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> >> @@ -95,7 +95,6 @@
> >> #define MATROX_DPMS_CLEARED (-1)
> >>
> >> #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
> >> -#define to_mga_encoder(x) container_of(x, struct mga_encoder, base)
> >> #define to_mga_connector(x) container_of(x, struct mga_connector, base)
> >>
> >> struct mga_crtc {
> >> @@ -110,12 +109,6 @@ struct mga_mode_info {
> >>struct mga_crtc *crtc;
> >> };
> >>
> >> -struct mga_encoder {
> >> - struct drm_encoder base;
> >> - int last_dpms;
> >> -};
> >> -
> >> -
> >> struct mga_i2c_chan {
> >>struct i2c_adapter adapter;
> >>struct drm_device *dev;
> >
> > Any particular reason why the drm_encoder is not embedded in struct
> > mga_device?
> >
> > I found it more elegant - like you did it for ast in the previous patch.
>
> I think I wanted something that uses drm_simple_encoder_create(). But I
> can change that. The embedded variant is indeed better.
You should consider to drop drm_simple_encoder_create() until there
is a driver that really needs it.
Sam
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 4/4] drm/qxl: Use simple encoder
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:15AM +0100, Thomas Zimmermann wrote:
> The qxl driver uses an empty implementation for its encoder. Replace
> the code with the generic simple encoder.
>
> v2:
> * rebase onto new simple-encoder interface
>
> Signed-off-by: Thomas Zimmermann
I looked at best_encoder - but could not see we could do anything.
So from browsing the code:
Acked-by: Sam Ravnborg
Sam
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 18 +++---
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ab4f8dd00400..9c0e1add59fb 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "qxl_drv.h"
> #include "qxl_object.h"
> @@ -1007,9 +1008,6 @@ static struct drm_encoder *qxl_best_encoder(struct
> drm_connector *connector)
> return _output->enc;
> }
>
> -static const struct drm_encoder_helper_funcs qxl_enc_helper_funcs = {
> -};
> -
> static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = {
> .get_modes = qxl_conn_get_modes,
> .mode_valid = qxl_conn_mode_valid,
> @@ -1059,15 +1057,6 @@ static const struct drm_connector_funcs
> qxl_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static void qxl_enc_destroy(struct drm_encoder *encoder)
> -{
> - drm_encoder_cleanup(encoder);
> -}
> -
> -static const struct drm_encoder_funcs qxl_enc_funcs = {
> - .destroy = qxl_enc_destroy,
> -};
> -
> static int qxl_mode_create_hotplug_mode_update_property(struct qxl_device
> *qdev)
> {
> if (qdev->hotplug_mode_update_property)
> @@ -1098,15 +1087,14 @@ static int qdev_output_init(struct drm_device *dev,
> int num_output)
> drm_connector_init(dev, _output->base,
> _connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
>
> - drm_encoder_init(dev, _output->enc, _enc_funcs,
> - DRM_MODE_ENCODER_VIRTUAL, NULL);
> + drm_simple_encoder_init(dev, _output->enc,
> + DRM_MODE_ENCODER_VIRTUAL);
>
> /* we get HPD via client monitors config */
> connector->polled = DRM_CONNECTOR_POLL_HPD;
> encoder->possible_crtcs = 1 << num_output;
> drm_connector_attach_encoder(_output->base,
> _output->enc);
> - drm_encoder_helper_add(encoder, _enc_helper_funcs);
> drm_connector_helper_add(connector, _connector_helper_funcs);
>
> drm_object_attach_property(>base,
> --
> 2.25.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> This patch makes the internal encoder implementation of the simple
> KMS helpers available to drivers.
>
> These simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
>
> v2:
> * move simple encoder to KMS helpers
> * remove name argument; simplifies implementation
> * don't allocate with devm_ interfaces; unsafe with DRM
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 -
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated _connector object and supporting optional _bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
> */
>
> -static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> .destroy = drm_encoder_cleanup,
> };
>
> +/**
> + * drm_simple_encoder_init - Initialize a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality. The
> + * encoder will be released automatically.
I got confused here. The comment says the encoder is automatically
released. But in this case we have a preallocated encoder (maybe
embedded in ast_private or something.
So the encoder is - as I understnad it - not released. But it is cleaned
up as it is also documented in the next sentences.
Sorry if I am dense, just returned from some travelling...
Sam
Settings for possible CRTC and
> + * clones are left to their initial values. The encoder will be cleaned up
> + * automatically as part of the mode-setting cleanup.
> + *
> + * Also see drm_simple_encoder_create().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type)
> +{
> + return drm_encoder_init(dev, encoder,
> + _simple_encoder_funcs_cleanup,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality.
> The
> + * encoder will be destroyed automatically as part of the mode-setting
> cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> +_simple_encoder_funcs_destroy,
> +encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +366,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> return ret;
>
> encoder->possible_crtcs =
Re: [Spice-devel] [PATCH v2 3/4] drm/mgag200: Use simple encoder
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:14AM +0100, Thomas Zimmermann wrote:
> The mgag200 driver uses an empty implementation for its encoder. Replace
> the code with the generic simple encoder.
>
> v2:
> * rebase onto new simple-encoder interface
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
> drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++
> 2 files changed, 3 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
> b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index aa32aad222c2..9bb9e8e14539 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -95,7 +95,6 @@
> #define MATROX_DPMS_CLEARED (-1)
>
> #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
> -#define to_mga_encoder(x) container_of(x, struct mga_encoder, base)
> #define to_mga_connector(x) container_of(x, struct mga_connector, base)
>
> struct mga_crtc {
> @@ -110,12 +109,6 @@ struct mga_mode_info {
> struct mga_crtc *crtc;
> };
>
> -struct mga_encoder {
> - struct drm_encoder base;
> - int last_dpms;
> -};
> -
> -
> struct mga_i2c_chan {
> struct i2c_adapter adapter;
> struct drm_device *dev;
Any particular reason why the drm_encoder is not embedded in struct
mga_device?
I found it more elegant - like you did it for ast in the previous patch.
I also noted there is one more unused "last_dpms" - but it is outside
the scope of this patch to remove it.
Sam
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 62a8e9ccb16d..957ea1057b6c 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include "mgag200_drv.h"
>
> @@ -1449,72 +1450,16 @@ static void mga_crtc_init(struct mga_device *mdev)
> drm_crtc_helper_add(_crtc->base, _helper_funcs);
> }
>
> -/*
> - * The encoder comes after the CRTC in the output pipeline, but before
> - * the connector. It's responsible for ensuring that the digital
> - * stream is appropriately converted into the output format. Setup is
> - * very simple in this case - all we have to do is inform qemu of the
> - * colour depth in order to ensure that it displays appropriately
> - */
> -
> -/*
> - * These functions are analagous to those in the CRTC code, but are intended
> - * to handle any encoder-specific limitations
> - */
> -static void mga_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> -{
> -
> -}
> -
> -static void mga_encoder_dpms(struct drm_encoder *encoder, int state)
> -{
> - return;
> -}
> -
> -static void mga_encoder_prepare(struct drm_encoder *encoder)
> -{
> -}
> -
> -static void mga_encoder_commit(struct drm_encoder *encoder)
> -{
> -}
> -
> -static void mga_encoder_destroy(struct drm_encoder *encoder)
> -{
> - struct mga_encoder *mga_encoder = to_mga_encoder(encoder);
> - drm_encoder_cleanup(encoder);
> - kfree(mga_encoder);
> -}
> -
> -static const struct drm_encoder_helper_funcs mga_encoder_helper_funcs = {
> - .dpms = mga_encoder_dpms,
> - .mode_set = mga_encoder_mode_set,
> - .prepare = mga_encoder_prepare,
> - .commit = mga_encoder_commit,
> -};
> -
> -static const struct drm_encoder_funcs mga_encoder_encoder_funcs = {
> - .destroy = mga_encoder_destroy,
> -};
> -
> static struct drm_encoder *mga_encoder_init(struct drm_device *dev)
> {
> struct drm_encoder *encoder;
> - struct mga_encoder *mga_encoder;
>
> - mga_encoder = kzalloc(sizeof(struct mga_encoder), GFP_KERNEL);
> - if (!mga_encoder)
> + encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
> + if (IS_ERR(encoder))
> return NULL;
>
> - encoder = _encoder->base;
> encoder->possible_crtcs = 0x1;
>
> - drm_encoder_init(dev, encoder, _encoder_encoder_funcs,
> - DRM_MODE_ENCODER_DAC, NULL);
> - drm_encoder_helper_add(encoder, _encoder_helper_funcs);
> -
> return encoder;
> }
>
> --
> 2.25.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> This patch makes the internal encoder implementation of the simple
> KMS helpers available to drivers.
>
> These simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
>
> v2:
> * move simple encoder to KMS helpers
> * remove name argument; simplifies implementation
> * don't allocate with devm_ interfaces; unsafe with DRM
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 -
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated _connector object and supporting optional _bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
This paragraph reads a bit strange to me - I read this as a
justification for addding a generic encoded that can be used by exisitg
drivers.
How about something like this:
Many drivers requires a very simple encoder that only fullfill
the minimum requirements of the display pipeline and do not add
any extra functionslity.
The simple-encoder functions drm_simple_encoder_init() and
drm_simple_encoder_create() provides an impålmentation of such
a simple encoder.
The simple encoder includes automatically release of resources.
And then leave it to the changelog to tell what should be done in
existing drivers.
> */
>
> -static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> .destroy = drm_encoder_cleanup,
> };
>
> +/**
> + * drm_simple_encoder_init - Initialize a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality. The
> + * encoder will be released automatically. Settings for possible CRTC and
> + * clones are left to their initial values. The encoder will be cleaned up
> + * automatically as part of the mode-setting cleanup.
> + *
> + * Also see drm_simple_encoder_create().
s/Also see/See also/??
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type)
> +{
> + return drm_encoder_init(dev, encoder,
> + _simple_encoder_funcs_cleanup,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality.
> The
> + * encoder will be destroyed automatically as part of the mode-setting
> cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
pointer-encoded?
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> +_simple_encoder_funcs_destroy,
> +encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
>
Re: [Spice-devel] [PATCH v2 2/4] drm/ast: Use simple encoder
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:13AM +0100, Thomas Zimmermann wrote:
> The ast driver uses an empty implementation for its encoder. Replace
> the code with the generic simple encoder.
>
> v2:
> * rebase onto new simple-encoder interface
>
> Signed-off-by: Thomas Zimmermann
>From browsign the code - looks good:
Acked-by: Sam Ravnborg
Sam
> ---
> drivers/gpu/drm/ast/ast_drv.h | 6 +-
> drivers/gpu/drm/ast/ast_mode.c | 25 -
> 2 files changed, 9 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index f5d8780776ae..656d591b154b 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -121,6 +121,7 @@ struct ast_private {
> unsigned int next_index;
> } cursor;
>
> + struct drm_encoder encoder;
> struct drm_plane primary_plane;
> struct drm_plane cursor_plane;
>
> @@ -238,13 +239,8 @@ struct ast_crtc {
> u8 offset_x, offset_y;
> };
>
> -struct ast_encoder {
> - struct drm_encoder base;
> -};
> -
> #define to_ast_crtc(x) container_of(x, struct ast_crtc, base)
> #define to_ast_connector(x) container_of(x, struct ast_connector, base)
> -#define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
>
> struct ast_vbios_stdtable {
> u8 misc;
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 562ea6d9df13..7a9f20a2fd30 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -40,6 +40,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ast_drv.h"
> #include "ast_tables.h"
> @@ -968,28 +969,18 @@ static int ast_crtc_init(struct drm_device *dev)
> * Encoder
> */
>
> -static void ast_encoder_destroy(struct drm_encoder *encoder)
> -{
> - drm_encoder_cleanup(encoder);
> - kfree(encoder);
> -}
> -
> -static const struct drm_encoder_funcs ast_enc_funcs = {
> - .destroy = ast_encoder_destroy,
> -};
> -
> static int ast_encoder_init(struct drm_device *dev)
> {
> - struct ast_encoder *ast_encoder;
> + struct ast_private *ast = dev->dev_private;
> + struct drm_encoder *encoder = >encoder;
> + int ret;
>
> - ast_encoder = kzalloc(sizeof(struct ast_encoder), GFP_KERNEL);
> - if (!ast_encoder)
> - return -ENOMEM;
> + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> + if (ret)
> + return ret;
>
> - drm_encoder_init(dev, _encoder->base, _enc_funcs,
> - DRM_MODE_ENCODER_DAC, NULL);
> + encoder->possible_crtcs = 1;
>
> - ast_encoder->base.possible_crtcs = 1;
> return 0;
> }
>
> --
> 2.25.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v1 09/33] drm/qxl: drop use of drmP.h
On Mon, Jul 01, 2019 at 08:38:43AM +0200, Gerd Hoffmann wrote: > On Sun, Jun 30, 2019 at 08:18:58AM +0200, Sam Ravnborg wrote: > > Drop use of the deprecated drmP.h header file. > > While touching the files divided includes in blocks, > > and when needed sort the blocks. > > Fix fallout. > > > > Signed-off-by: Sam Ravnborg > > Cc: Dave Airlie > > Cc: Gerd Hoffmann > > Cc: virtualizat...@lists.linux-foundation.org > > Cc: spice-devel@lists.freedesktop.org > > Acked-by: Gerd Hoffmann Thanks, applied. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v1 09/33] drm/qxl: drop use of drmP.h
Drop use of the deprecated drmP.h header file.
While touching the files divided includes in blocks,
and when needed sort the blocks.
Fix fallout.
Signed-off-by: Sam Ravnborg
Cc: Dave Airlie
Cc: Gerd Hoffmann
Cc: virtualizat...@lists.linux-foundation.org
Cc: spice-devel@lists.freedesktop.org
---
The list of cc: was too large to add all recipients to the cover letter.
Please find cover letter here:
https://lists.freedesktop.org/archives/dri-devel/2019-June/thread.html
Search for "drm: drop use of drmp.h in drm-misc"
Sam
drivers/gpu/drm/qxl/qxl_cmd.c | 2 ++
drivers/gpu/drm/qxl/qxl_debugfs.c | 4 ++--
drivers/gpu/drm/qxl/qxl_display.c | 3 +++
drivers/gpu/drm/qxl/qxl_draw.c| 2 ++
drivers/gpu/drm/qxl/qxl_drv.c | 10 +++---
drivers/gpu/drm/qxl/qxl_drv.h | 7 +++
drivers/gpu/drm/qxl/qxl_gem.c | 1 -
drivers/gpu/drm/qxl/qxl_ioctl.c | 3 +++
drivers/gpu/drm/qxl/qxl_irq.c | 4
drivers/gpu/drm/qxl/qxl_kms.c | 9 ++---
drivers/gpu/drm/qxl/qxl_release.c | 6 +-
drivers/gpu/drm/qxl/qxl_ttm.c | 16 +---
12 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 0a2e51af1230..ac1081f55b51 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -25,6 +25,8 @@
/* QXL cmd/ring handling */
+#include
+
#include
#include "qxl_drv.h"
diff --git a/drivers/gpu/drm/qxl/qxl_debugfs.c
b/drivers/gpu/drm/qxl/qxl_debugfs.c
index 118422549828..a85ec100b0cc 100644
--- a/drivers/gpu/drm/qxl/qxl_debugfs.c
+++ b/drivers/gpu/drm/qxl/qxl_debugfs.c
@@ -28,9 +28,9 @@
* Alon Levy
*/
-#include
+#include
+#include
-#include
#include "qxl_drv.h"
#include "qxl_object.h"
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 8b319ebbb0fb..023fb5a69af1 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -24,11 +24,14 @@
*/
#include
+#include
+
#include
#include
#include
#include
#include
+#include
#include "qxl_drv.h"
#include "qxl_object.h"
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 97c3f1a95a32..5bebf1ea1c5d 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -20,6 +20,8 @@
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
+#include
+
#include "qxl_drv.h"
#include "qxl_object.h"
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index d8f64886474b..b57a37543613 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -28,14 +28,18 @@
*Alon Levy
*/
-#include
+#include "qxl_drv.h"
#include
+#include
+#include
-#include
#include
+#include
+#include
#include
+#include
#include
-#include "qxl_drv.h"
+
#include "qxl_object.h"
static const struct pci_device_id pciidlist[] = {
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 2896bb6fdbf4..ae82e5fab09c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -31,22 +31,21 @@
*/
#include
-#include
#include
#include
+#include
#include
#include
#include
+#include
#include
-#include
+#include
#include
#include
-/* just for ttm_validate_buffer */
#include
#include
#include
-#include
#include "qxl_dev.h"
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 89606c819d82..89e4f9a7249c 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -23,7 +23,6 @@
* Alon Levy
*/
-#include
#include
#include "qxl_drv.h"
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index d410e2925162..8117a45b3610 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -23,6 +23,9 @@
* Alon Levy
*/
+#include
+#include
+
#include "qxl_drv.h"
#include "qxl_object.h"
diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
index 3bb31add6350..8435af108632 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -23,6 +23,10 @@
* Alon Levy
*/
+#include
+
+#include
+
#include "qxl_drv.h"
irqreturn_t qxl_irq_handler(int irq, void *arg)
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index bee61fa2c9bc..611cbe7aee69 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -23,11 +23,14 @@
* Alon Levy
*/
-#include "qxl_drv.h"
-#include "qxl_object.h"
+#include
+#include
+#include
#include
-#include
+
+#include "qxl_drv.h"
+#include "qxl_object.h"
int qxl_lo
Re: [Spice-devel] [PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere
Hi Daniel.
Minor nitpick..
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 65d599065709..4fd09a9ad67a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3193,7 +3193,7 @@ static struct drm_driver driver = {
>* deal with them for Intel hardware.
>*/
> .driver_features =
> - DRIVER_GEM | DRIVER_PRIME |
> + DRIVER_GEM |
> DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ,
Adds a whitespace.
Sam
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code
Hi Noralf. > > Lovely diffstat, thanks to the new generic fbdev emulation. > > > > drm/qxl/Makefile |2 > > drm/qxl/qxl_draw.c | 232 > > drm/qxl/qxl_drv.h | 21 --- > > drm/qxl/qxl_fb.c | 300 > > - > > > > Signed-off-by: Gerd Hoffmann > > --- > > drivers/gpu/drm/qxl/qxl_drv.h | 21 --- > > drivers/gpu/drm/qxl/qxl_draw.c | 232 --- > > drivers/gpu/drm/qxl/qxl_fb.c | 300 > > - > > drivers/gpu/drm/qxl/Makefile | 2 +- > > 4 files changed, 1 insertion(+), 554 deletions(-) > > delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c > > > > Nice! > > Acked-by: Noralf Trønnes It must be a very satisfying experience to see such benefits of the many hours of works you have put into the genric fbdev emulation code. Well done, and indeed a nice patch from Gerd. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h
Hi Daniel. On Thu, Jan 17, 2019 at 10:03:34PM +0100, 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. How are the plans to get this patchset merged? There were dependencies on onging drmP.h removal in i915 IIRC? I guess my "Minimize drmP.h dependencies" patch-set also have a role in this. This does not hold up any work of mine, mainly wanted to make sure this was not lost between all the other patches. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h
Hi Daniel et al. > > > > Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy > > kms drivers. Just removing it from all the atomic drivers caused lots of > > fallout, I expect even more if you entirely remove the includes it has. > > Maybe a todo, care to pls create that patch since it's your idea? > > The main reason I bailed out initially was that this would create > small changes to several otherwise seldomly touched files. > And then we would later come and remove drmP.h - so lots of > small but incremental changes to the same otherwise seldomly > edited files. > And the job was only partially done. > > I will try to experiment with an approach where I clean up the > include/drm/*.h files a little (like suggested above, +delete drmP.h > and maybe a bit more). > > Then to try on a driver by driver basis to make it build with a > cleaned set of include files. > I hope that the cleaned up driver can still build without the > cleaned header files so the changes can be submitted piecemal. > > Will do so with an eye on the lesser maintained drivers to try it > out to avoid creating too much chrunch for others. I have now a few patches queued, but the result is not too pretty. I did the following: - For all files in include/drm/*.h the set of include files were adjusted to the minimum number of files required to make them build without any other files included first. Created one .c file for each .h file. Then included the .h file and adjusted to the minimal set of include files. In the process a lot of forwards were added. - Deleted drmP.h - Fixed build of a few drivers: sti, tilcdc, gma500, tve200, via Some observations: - Killing all the includes not needed in the headers files results in a a lot of extra changes. Examples: drm_modseset_helper_vtables.h is no longer included by anyone, so needs to be added in many files drm_atomic_state_helper.h is no longer included by anyone so likewise needs to be added in many files - It is very tedious to do this properly. The process I followed was: - delete / comment out all include files - add back the obvious from a quick scan of the code - build - fix - build - fix - build - fix ... - next file... - The result is errorprone as only the allyesconfig + allmodconfig variants are tested. But reallife configurations are more diverse. Current diffstat: 111 files changed, 771 insertions(+), 401 deletions(-) This is for the 5 drivers alone and not the header cleanup. So long story short - this is not good and not the way forward. I will try to come up with a few improvements to make the headers files selfcontained, but restricted to the changes that add forwards/include to avoid the chrunch in all the drivers. And then post for review a few patches to clean up some headers. If the cleanup gets a go I will try to persuade the introduction of these. This will include, but will not be limited to, the above mentioned drm_crtc_helper.h header file. For now too much time was already spent on this, so it is at the moment pushed back on my TODO list. This mail serve also as a kind of "where had I left", when/if I pick this up again. If there are anyone that knows some tooling that can help in the process of adjusting the header files I am all ears. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h
On Thu, Jan 17, 2019 at 05:45:41PM +0100, Daniel Vetter wrote: > On Wed, Jan 16, 2019 at 07:10:18PM +0100, Sam Ravnborg wrote: > > Hi Daniel. > > > > > v5: Actually try to sort them, and while at it, sort all the ones I > > > touch. > > > > Applied this variant on top of drm-misc and did a build test. > > Looked good for ia64, x86 and alpha. > > > > Took a closer look at the changes to atmel_hlcd - and they looked OK. > > > > But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and > > drm_kms_helper_poll_fini(). > > But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe > > have a driver here where we have plugged the drm_poll infrastructure, > > but it is not in use. > > > > > include/drm/drm_crtc_helper.h | 16 --- > > > > The list of include files in this file could be dropped and replaced by: > > struct drm_connector; > > struct drm_device; > > struct drm_display_mode; > > struct drm_encoder; > > struct drm_framebuffer; > > struct drm_mode_set; > > struct drm_modeset_acquire_ctx; > > > > I tried to do so on top of your patch. > > But there were too many build errros and I somehow lost the motivation. > > Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy > kms drivers. Just removing it from all the atomic drivers caused lots of > fallout, I expect even more if you entirely remove the includes it has. > Maybe a todo, care to pls create that patch since it's your idea? The main reason I bailed out initially was that this would create small changes to several otherwise seldomly touched files. And then we would later come and remove drmP.h - so lots of small but incremental changes to the same otherwise seldomly edited files. And the job was only partially done. I will try to experiment with an approach where I clean up the include/drm/*.h files a little (like suggested above, +delete drmP.h and maybe a bit more). Then to try on a driver by driver basis to make it build with a cleaned set of include files. I hope that the cleaned up driver can still build without the cleaned header files so the changes can be submitted piecemal. Will do so with an eye on the lesser maintained drivers to try it out to avoid creating too much chrunch for others. And if it works out I expect the active drivers to follow the example. todo.rst item will wait until I run out of energy. Sam ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h
Hi Daniel. > v5: Actually try to sort them, and while at it, sort all the ones I > touch. Applied this variant on top of drm-misc and did a build test. Looked good for ia64, x86 and alpha. Took a closer look at the changes to atmel_hlcd - and they looked OK. But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and drm_kms_helper_poll_fini(). But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe have a driver here where we have plugged the drm_poll infrastructure, but it is not in use. > include/drm/drm_crtc_helper.h | 16 --- The list of include files in this file could be dropped and replaced by: struct drm_connector; struct drm_device; struct drm_display_mode; struct drm_encoder; struct drm_framebuffer; struct drm_mode_set; struct drm_modeset_acquire_ctx; I tried to do so on top of your patch. But there were too many build errros and I somehow lost the motivation. > include/drm/drm_probe_helper.h| 27 +++ This on the other hand is fine - as expected as this is a new file. But the above is just some random comments so: Acked-by: Sam Ravnborg ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
