Hi, Thanks for the review! On 03-06-16 19:27, Thierry Reding wrote:
On Fri, Jun 03, 2016 at 03:06:32PM +0200, Hans de Goede wrote:Grain-media GM12U320 based devices are mini video projectors using USB for both power and video data transport. This commit adds a kms driver for these devices, including prime support. This driver is based on the existing udl kms driver, and the gm12u320 fb driver by Viacheslav Nurmekhamitov <slavrn@xxxxxxxxx>. Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> --- Changes in v2: -Rebase on 4.7-rc1 -Sync with udl, bring in any fixes done to udl since v1This sounds a little nightmarish in the long term. From a cursory look this duplicates a bunch of code that's present in UDL, where the main differences are EDID handling and data transfer. I wonder if we can't find a better way of reusing code than duplicating it.
I was planning on eventually moving the shared code to some kms-usb helper lib, I was thinking it would be good to have more then one user first, but if people prefer me first factoring out some code from udl into a lib I can do that instead.
diff --git a/drivers/gpu/drm/gm12u320/gm12u320_connector.c b/drivers/gpu/drm/gm12u320/gm12u320_connector.c[...]+/* + * Note this assumes this driver is only ever used with the Acer C120, if we + * add support for other devices the vendor and model should be parameterized. + */ +static struct edid gm12u320_edid = { + .header = { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00 }, + .mfg_id = { 0x04, 0x72 }, /* "ACR" */ + .prod_code = { 0x20, 0xc1 }, /* C120h */ + .mfg_week = 1, + .mfg_year = 1, + .version = 1, /* EDID 1.3 */ + .revision = 3, /* EDID 1.3 */ + .input = 0x80, /* Digital input */ + .features = 0x02, /* Pref timing in DTD 1 */ + .standard_timings = { { 1, 1 }, { 1, 1 }, { 1, 1 }, { 1, 1 }, + { 1, 1 }, { 1, 1 }, { 1, 1 }, { 1, 1 } }, + .detailed_timings = { { + .pixel_clock = 3383, + /* hactive = 852, hblank = 256 */ + .data.pixel_data.hactive_lo = 0x54, + .data.pixel_data.hblank_lo = 0x00, + .data.pixel_data.hactive_hblank_hi = 0x31, + /* vactive = 480, vblank = 28 */ + .data.pixel_data.vactive_lo = 0xe0, + .data.pixel_data.vblank_lo = 0x1c, + .data.pixel_data.vactive_vblank_hi = 0x10, + /* hsync offset 40 pw 128, vsync offset 1 pw 4 */ + .data.pixel_data.hsync_offset_lo = 0x28, + .data.pixel_data.hsync_pulse_width_lo = 0x80, + .data.pixel_data.vsync_offset_pulse_width_lo = 0x14, + .data.pixel_data.hsync_vsync_offset_pulse_width_hi = 0x00, + /* Digital separate syncs, hsync+, vsync+ */ + .data.pixel_data.misc = 0x1e, + }, { + .pixel_clock = 0, + .data.other_data.type = 0xfd, /* Monitor ranges */ + .data.other_data.data.range.min_vfreq = 59, + .data.other_data.data.range.max_vfreq = 61, + .data.other_data.data.range.min_hfreq_khz = 29, + .data.other_data.data.range.max_hfreq_khz = 32, + .data.other_data.data.range.pixel_clock_mhz = 4, /* 40 MHz */ + .data.other_data.data.range.flags = 0, + .data.other_data.data.range.formula.cvt = { + 0xa0, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20 }, + }, { + .pixel_clock = 0, + .data.other_data.type = 0xfc, /* Model string */ + .data.other_data.data.str.str = { + 'C', '1', '2', '0', 'P', 'r', 'o', 'j', 'e', 'c', + 't', 'o', 'r' }, + }, { + .pixel_clock = 0, + .data.other_data.type = 0xfe, /* Unspecified text / padding */ + .data.other_data.data.str.str = { + '\n', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', + ' ', ' ', ' ' }, + } }, + .checksum = 0x40, +};Where did you get this from?
I completely made it up, other then for the resolution which is the native resolution of the lcd in the projector
Doesn't this device have a chip that you can query at runtime?
Not to my knowledge.
+static int gm12u320_connector_set_property(struct drm_connector *connector, + struct drm_property *property, + uint64_t val) +{ + return 0; +}Just drop it if it doesn't do anything.
Ok, then it should probably be dropped from udl too, I'll check and submit a separate patch for this.
+int gm12u320_connector_init(struct drm_device *dev, + struct drm_encoder *encoder) +{ + struct drm_connector *connector; + + connector = kzalloc(sizeof(struct drm_connector), GFP_KERNEL); + if (!connector) + return -ENOMEM; + + drm_connector_init(dev, connector, &gm12u320_connector_funcs, + DRM_MODE_CONNECTOR_Unknown);According to the Grain Media website this chip is a USB 3.0-to-HDMI bridge, so DRM_MODE_CONNECTOR_HDMIA might be a better choice here.
Hmm, where did you find this ? Note I've never actually verified the gm12u320 name this comes from previous reverse engineering work done on the projector. HDMI for what is in essence a lcd panel seems wrong, maybe lvds if you want to avoid Unknown ?
diff --git a/drivers/gpu/drm/gm12u320/gm12u320_drv.c b/drivers/gpu/drm/gm12u320/gm12u320_drv.c[...]new file mode 100644 index 0000000..eff3a44 --- /dev/null +++ b/drivers/gpu/drm/gm12u320/gm12u320_drv.c @@ -0,0 +1,160 @@ +/* + * Copyright (C) 2012-2016 Red Hat Inc. + * + * This file is subject to the terms and conditions of the GNU General Public + * License v2. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include <linux/module.h> +#include <drm/drmP.h> +#include <drm/drm_crtc_helper.h> +#include "gm12u320_drv.h" + +static int gm12u320_driver_set_busid(struct drm_device *d, struct drm_master *m) +{ + return 0; +}This is optional, you can drop it.
Idem. as above, then it should probably be dropped from udl too, I'll check and submit a separate patch for this. Regards, Hans _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
