The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++---------------- drivers/gpu/drm/udl/udl_connector.c | 8 ++--- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 +++++---- include/drm/drm_device.h | 21 ++++++++---- 6 files changed, 52 insertions(+), 47 deletions(-)
-- 2.28.0
We have DRM drivers that operate on USB devices. So far they had to store a pointer to the USB device structure. Move the reference into struct drm_device. Putting the USB device into a union with the PCI data saves a few bytes. Both should mutually exclusive.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- include/drm/drm_device.h | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index f4f68e7a9149..9871fcabd720 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -25,6 +25,7 @@ struct inode; struct pci_dev; struct pci_controller;
+struct usb_device;
/** * enum drm_switch_power - power state of drm device @@ -283,16 +284,24 @@ struct drm_device { */ spinlock_t event_lock;
- /** @agp: AGP data */ - struct drm_agp_head *agp; + union { + struct { + /** @agp: AGP data */ + struct drm_agp_head *agp;
- /** @pdev: PCI device structure */ - struct pci_dev *pdev; + /** @pdev: PCI device structure */ + struct pci_dev *pdev;
#ifdef __alpha__ - /** @hose: PCI hose, only used on ALPHA platforms. */ - struct pci_controller *hose; + /** @hose: PCI hose, only used on ALPHA platforms. */ + struct pci_controller *hose; #endif + }; + + /** @udev: USB device structure */ + struct usb_device *udev; + }; + /** @num_crtcs: Number of CRTCs on this device */ unsigned int num_crtcs;
Drop the driver's udev field in favor of the one in struct drm_device. No functional changes made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++++------------------ 1 file changed, 24 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index cc397671f689..7d98906b3d59 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -45,7 +45,7 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)"); #define GM12U320_BLOCK_COUNT 20
#define GM12U320_ERR(fmt, ...) \ - DRM_DEV_ERROR(&gm12u320->udev->dev, fmt, ##__VA_ARGS__) + DRM_DEV_ERROR(&gm12u320->dev.udev->dev, fmt, ##__VA_ARGS__)
#define MISC_RCV_EPT 1 #define DATA_RCV_EPT 2 @@ -85,7 +85,6 @@ struct gm12u320_device { struct drm_device dev; struct drm_simple_display_pipe pipe; struct drm_connector conn; - struct usb_device *udev; unsigned char *cmd_buf; unsigned char *data_buf[GM12U320_BLOCK_COUNT]; struct { @@ -191,6 +190,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320, u8 req_a, u8 req_b, u8 arg_a, u8 arg_b, u8 arg_c, u8 arg_d) { + struct usb_device *udev = gm12u320->dev.udev; int ret, len;
memcpy(gm12u320->cmd_buf, &cmd_misc, CMD_SIZE); @@ -202,8 +202,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320, gm12u320->cmd_buf[25] = arg_d;
/* Send request */ - ret = usb_bulk_msg(gm12u320->udev, - usb_sndbulkpipe(gm12u320->udev, MISC_SND_EPT), + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, MISC_SND_EPT), gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT); if (ret || len != CMD_SIZE) { GM12U320_ERR("Misc. req. error %d\n", ret); @@ -211,8 +210,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320, }
/* Read value */ - ret = usb_bulk_msg(gm12u320->udev, - usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT), + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT), gm12u320->cmd_buf, MISC_VALUE_SIZE, &len, DATA_TIMEOUT); if (ret || len != MISC_VALUE_SIZE) { @@ -222,8 +220,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320, /* cmd_buf[0] now contains the read value, which we don't use */
/* Read status */ - ret = usb_bulk_msg(gm12u320->udev, - usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT), + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT), gm12u320->cmd_buf, READ_STATUS_SIZE, &len, CMD_TIMEOUT); if (ret || len != READ_STATUS_SIZE) { @@ -331,6 +328,7 @@ static void gm12u320_fb_update_work(struct work_struct *work) struct gm12u320_device *gm12u320 = container_of(to_delayed_work(work), struct gm12u320_device, fb_update.work); + struct usb_device *udev = gm12u320->dev.udev; int block, block_size, len; int ret = 0;
@@ -350,43 +348,41 @@ static void gm12u320_fb_update_work(struct work_struct *work) gm12u320->cmd_buf[21] = block | (gm12u320->fb_update.frame << 7);
- ret = usb_bulk_msg(gm12u320->udev, - usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT), - gm12u320->cmd_buf, CMD_SIZE, &len, - CMD_TIMEOUT); + ret = usb_bulk_msg(udev, + usb_sndbulkpipe(udev, DATA_SND_EPT), + gm12u320->cmd_buf, CMD_SIZE, &len, + CMD_TIMEOUT); if (ret || len != CMD_SIZE) goto err;
/* Send data block to device */ - ret = usb_bulk_msg(gm12u320->udev, - usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT), - gm12u320->data_buf[block], block_size, - &len, DATA_TIMEOUT); + ret = usb_bulk_msg(udev, + usb_sndbulkpipe(udev, DATA_SND_EPT), + gm12u320->data_buf[block], block_size, + &len, DATA_TIMEOUT); if (ret || len != block_size) goto err;
/* Read status */ - ret = usb_bulk_msg(gm12u320->udev, - usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT), - gm12u320->cmd_buf, READ_STATUS_SIZE, &len, - CMD_TIMEOUT); + ret = usb_bulk_msg(udev, + usb_rcvbulkpipe(udev, DATA_RCV_EPT), + gm12u320->cmd_buf, READ_STATUS_SIZE, &len, + CMD_TIMEOUT); if (ret || len != READ_STATUS_SIZE) goto err; }
/* Send draw command to device */ memcpy(gm12u320->cmd_buf, cmd_draw, CMD_SIZE); - ret = usb_bulk_msg(gm12u320->udev, - usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT), - gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT); + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, DATA_SND_EPT), + gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT); if (ret || len != CMD_SIZE) goto err;
/* Read status */ - ret = usb_bulk_msg(gm12u320->udev, - usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT), - gm12u320->cmd_buf, READ_STATUS_SIZE, &len, - gm12u320->fb_update.draw_status_timeout); + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, DATA_RCV_EPT), + gm12u320->cmd_buf, READ_STATUS_SIZE, &len, + gm12u320->fb_update.draw_status_timeout); if (ret || len != READ_STATUS_SIZE) goto err;
@@ -638,7 +634,7 @@ static int gm12u320_usb_probe(struct usb_interface *interface, if (IS_ERR(gm12u320)) return PTR_ERR(gm12u320);
- gm12u320->udev = interface_to_usbdev(interface); + gm12u320->dev.udev = interface_to_usbdev(interface); INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work); mutex_init(&gm12u320->fb_update.lock);
Drop the driver's udev field in favor of the one in struct drm_device. No functional changes made.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/udl/udl_connector.c | 8 ++++---- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 ++++++++------- 4 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c index cdc1c42e1669..b86e75d76c5a 100644 --- a/drivers/gpu/drm/udl/udl_connector.c +++ b/drivers/gpu/drm/udl/udl_connector.c @@ -20,6 +20,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, int ret, i; u8 *read_buff; struct udl_device *udl = data; + struct usb_device *udev = udl->drm.udev;
read_buff = kmalloc(2, GFP_KERNEL); if (!read_buff) @@ -27,10 +28,9 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
for (i = 0; i < len; i++) { int bval = (i + block * EDID_LENGTH) << 8; - ret = usb_control_msg(udl->udev, - usb_rcvctrlpipe(udl->udev, 0), - (0x02), (0x80 | (0x02 << 5)), bval, - 0xA1, read_buff, 2, HZ); + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), + 0x02, (0x80 | (0x02 << 5)), bval, + 0xA1, read_buff, 2, HZ); if (ret < 1) { DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret); kfree(read_buff); diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 96d4317a2c1b..0aca9a3221ab 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -62,7 +62,7 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) if (IS_ERR(udl)) return udl;
- udl->udev = udev; + udl->drm.udev = udev;
r = udl_init(udl); if (r) diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index b1461f30780b..889bfa21deb0 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -50,7 +50,6 @@ struct urb_list { struct udl_device { struct drm_device drm; struct device *dev; - struct usb_device *udev;
struct drm_simple_display_pipe display_pipe;
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index f5d27f2a5654..f2ef5b169523 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -98,19 +98,19 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev, */ static int udl_select_std_channel(struct udl_device *udl) { - int ret; static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7, 0x1C, 0x88, 0x5E, 0x15, 0x60, 0xFE, 0xC6, 0x97, 0x16, 0x3D, 0x47, 0xF2}; + struct usb_device *udev = udl->drm.udev; void *sendbuf; + int ret;
sendbuf = kmemdup(set_def_chn, sizeof(set_def_chn), GFP_KERNEL); if (!sendbuf) return -ENOMEM;
- ret = usb_control_msg(udl->udev, - usb_sndctrlpipe(udl->udev, 0), + ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), NR_USB_REQUEST_CHANNEL, (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0, sendbuf, sizeof(set_def_chn), @@ -198,6 +198,7 @@ static void udl_free_urb_list(struct drm_device *dev) static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size) { struct udl_device *udl = to_udl(dev); + struct usb_device *udev = udl->drm.udev; struct urb *urb; struct urb_node *unode; char *buf; @@ -229,7 +230,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size) } unode->urb = urb;
- buf = usb_alloc_coherent(udl->udev, size, GFP_KERNEL, + buf = usb_alloc_coherent(udev, size, GFP_KERNEL, &urb->transfer_dma); if (!buf) { kfree(unode); @@ -243,8 +244,8 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size) }
/* urb->transfer_buffer_length set to actual before submit */ - usb_fill_bulk_urb(urb, udl->udev, usb_sndbulkpipe(udl->udev, 1), - buf, size, udl_urb_completion, unode); + usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, 1), + buf, size, udl_urb_completion, unode); urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
list_add_tail(&unode->entry, &udl->urbs.list); @@ -316,7 +317,7 @@ int udl_init(struct udl_device *udl)
mutex_init(&udl->gem_lock);
- if (!udl_parse_vendor_descriptor(dev, udl->udev)) { + if (!udl_parse_vendor_descriptor(dev, dev->udev)) { ret = -ENODEV; DRM_ERROR("firmware not recognized. Assume incompatible device\n"); goto err;
On Wed, Oct 21, 2020 at 03:07:29PM +0200, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Uh no.
If you're really concerned about the 8 bytes wasted, use drm_device->dev instead, and upcast it to the usb device. I think some drivers do this already (at least on the platform side iirc).
But we had this entire drm_bus midlayer stuff already, and it took forever to sunset it. I don't want to go back, and saving 8 bytes in a giantic structure isn't worth that risk imo. -Daniel
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++---------------- drivers/gpu/drm/udl/udl_connector.c | 8 ++--- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 +++++---- include/drm/drm_device.h | 21 ++++++++---- 6 files changed, 52 insertions(+), 47 deletions(-)
-- 2.28.0
On Wed, Oct 21, 2020 at 10:01:29PM +0200, Daniel Vetter wrote:
On Wed, Oct 21, 2020 at 03:07:29PM +0200, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Uh no.
If you're really concerned about the 8 bytes wasted, use drm_device->dev instead, and upcast it to the usb device. I think some drivers do this already (at least on the platform side iirc).
But we had this entire drm_bus midlayer stuff already, and it took forever to sunset it. I don't want to go back, and saving 8 bytes in a giantic structure isn't worth that risk imo.
Typing this I realized that we could even move the pdev pointer to the legacy chunk of drm_device. Instead of checking for drm_device->pdev we could instead check for dev_is_pci(drm_device->dev) in the 1-2 core code places.
But it's a pile of churn to roll that out to drivers, and I'm not sure the 8 bytes saved is even close to paying for that huge effort. -Daniel
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++---------------- drivers/gpu/drm/udl/udl_connector.c | 8 ++--- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 +++++---- include/drm/drm_device.h | 21 ++++++++---- 6 files changed, 52 insertions(+), 47 deletions(-)
-- 2.28.0
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Regards,
Hans
drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++---------------- drivers/gpu/drm/udl/udl_connector.c | 8 ++--- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 +++++---- include/drm/drm_device.h | 21 ++++++++---- 6 files changed, 52 insertions(+), 47 deletions(-)
-- 2.28.0
Hi
On 22.10.20 11:20, Hans de Goede wrote:
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead do an upcast from drm_device.dev to the USB device structure. The driver patches 2 and 3 will be slightly different. Unless you object, I''ll take the r-b into the new patches.
Best regards Thomas
Regards,
Hans
drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++---------------- drivers/gpu/drm/udl/udl_connector.c | 8 ++--- drivers/gpu/drm/udl/udl_drv.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 1 - drivers/gpu/drm/udl/udl_main.c | 15 +++++---- include/drm/drm_device.h | 21 ++++++++---- 6 files changed, 52 insertions(+), 47 deletions(-)
-- 2.28.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On 10/22/20 11:30 AM, Thomas Zimmermann wrote:
Hi
On 22.10.20 11:20, Hans de Goede wrote:
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead do an upcast from drm_device.dev to the USB device structure. The driver patches 2 and 3 will be slightly different. Unless you object, I''ll take the r-b into the new patches.
I somehow missed Daniel's reply about this.
With that said, hmm that is going to be an interesting up-cast, at least for the gm12u320, that is going to look something like this:
struct usb_device *udev = interface_to_usbdev(to_usb_interface(drm_dev->dev));
(I wrote drm_dev instead of dev to make it more clear what is going on)
For the DRM_DEV_ERROR() macro you can just use gm12u320->dev.dev , that will make the errors be printed with the in usb-interface device-name as prefix instead of the usb-device device-name, but that is fine.
I wonder of this is all worth it then though, just to save those few bytes ?
The first version made some sense since it made how drm devices with usb resp. pci parents are handled consistent. Now it seems to make the code somewhat harder to understand just to save the storage for a single pointer...
Regards,
Hans
Hi Hans
On 22.10.20 12:17, Hans de Goede wrote:
Hi,
On 10/22/20 11:30 AM, Thomas Zimmermann wrote:
Hi
On 22.10.20 11:20, Hans de Goede wrote:
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead do an upcast from drm_device.dev to the USB device structure. The driver patches 2 and 3 will be slightly different. Unless you object, I''ll take the r-b into the new patches.
I somehow missed Daniel's reply about this.
With that said, hmm that is going to be an interesting up-cast, at least for the gm12u320, that is going to look something like this:
struct usb_device *udev = interface_to_usbdev(to_usb_interface(drm_dev->dev));
(I wrote drm_dev instead of dev to make it more clear what is going on)
For the DRM_DEV_ERROR() macro you can just use gm12u320->dev.dev , that will make the errors be printed with the in usb-interface device-name as prefix instead of the usb-device device-name, but that is fine.
I wonder of this is all worth it then though, just to save those few bytes ?
Daniel and I briefly discussed on IRC if we could make drm_device.pdev (the PCI dev ) a legacy field in the longer term. All Linux devices would be retrieved from drm_device.dev then.
The first version made some sense since it made how drm devices with usb resp. pci parents are handled consistent. Now it seems to make the code somewhat harder to understand just to save the storage for a single pointer...
What's the implication of setting drm_device.dev to the value of struct usb_device.dev ? That's the device all the code interacts with.
Best regards Thomas
Regards,
Hans
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On 10/22/20 12:57 PM, Thomas Zimmermann wrote:
Hi Hans
On 22.10.20 12:17, Hans de Goede wrote:
Hi,
On 10/22/20 11:30 AM, Thomas Zimmermann wrote:
Hi
On 22.10.20 11:20, Hans de Goede wrote:
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead do an upcast from drm_device.dev to the USB device structure. The driver patches 2 and 3 will be slightly different. Unless you object, I''ll take the r-b into the new patches.
I somehow missed Daniel's reply about this.
With that said, hmm that is going to be an interesting up-cast, at least for the gm12u320, that is going to look something like this:
struct usb_device *udev = interface_to_usbdev(to_usb_interface(drm_dev->dev));
(I wrote drm_dev instead of dev to make it more clear what is going on)
For the DRM_DEV_ERROR() macro you can just use gm12u320->dev.dev , that will make the errors be printed with the in usb-interface device-name as prefix instead of the usb-device device-name, but that is fine.
I wonder of this is all worth it then though, just to save those few bytes ?
Daniel and I briefly discussed on IRC if we could make drm_device.pdev (the PCI dev ) a legacy field in the longer term. All Linux devices would be retrieved from drm_device.dev then.
I see. Then I guess the fancy cast which I gave above is the right way to move forward with this.
The first version made some sense since it made how drm devices with usb resp. pci parents are handled consistent. Now it seems to make the code somewhat harder to understand just to save the storage for a single pointer...
What's the implication of setting drm_device.dev to the value of struct usb_device.dev ? That's the device all the code interacts with.
USB drivers bind to USB interfaces, not to the USB device (the parent of the interfaces). A single USB device may have multiple interfaces, e.g. an USB headset may implement the USB audio class for the microphone and headphones function, while having a second interface implementing the HID interface for things like volume buttons, a play/pause button, etc.
So from the USB device model's pov making the USB device itself the parent of the drm device is just wrong. I guess it may not cause much issues, but it very much goes against how USB devices / interfaces are supposed to be used inside the kernel.
So I guess we should just move forward with a v2 with the long / fancy casts.
As for keeping my Reviewed-by for that v2, yes that is fine.
Regards,
Hans
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Hans de Goede -
Thomas Zimmermann