Re: [PATCH v2] drm: Add Grain Media GM12U320 kms driver

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 03, 2016 at 07:58:57PM +0200, Hans de Goede wrote:
> 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@yandex.ru>.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v2:
> > > -Rebase on 4.7-rc1
> > > -Sync with udl, bring in any fixes done to udl since v1
> > 
> > This 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.

Yeah I think waiting for more usb drivers is probably better. I don't want
to resurrect all the drm_bus mistakes once more, we're still working to
get rid of them all.

The other bit: Can I haz this in atomic, plz?

And when you look at atomic, it would probably be worth it to check out
Noralf's drm_simple_display_pipe helpers. They're made exactly for dumb
devices like this one here. It would cut out a lof of code.

The one thing you might need on top of Noralf's patches is some
super-simple support for cursors. That should be easy to add though.
-Daniel


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

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