[PATCH 0/3] drm: Store USB device in struct drm_device

[PATCH 0/3] drm: Store USB device in struct drm_device

[Thomas Zimmermann]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: Add reference to USB device to struct drm_device

[Thomas Zimmermann]

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;
 
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/tiny/gm12u320: Store USB device in struct drm_device.udev

[Thomas Zimmermann]

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);
 
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/udl: Store USB device in struct drm_device.udev

[Thomas Zimmermann]

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;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Hans de Goede]

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
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Thomas Zimmermann]

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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Hans de Goede]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Thomas Zimmermann]

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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Store USB device in struct drm_device

[Hans de Goede]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel