Message ID | 20130625204726.BF65C12647A@dev.laptop.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 25 Jun 2013 16:47:26 -0400 (EDT) Daniel Drake <dsd@laptop.org> wrote: > Hi Russell, > > Thanks a lot for writing the Armada DRM driver. > > I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 > aka PXA2128). After a bit of fighting, I have it running. Could you share your > X driver, or your methodology for testing hardware cursors? I'd like to test > your work there too. > > It's probably easiest to get your cubox driver merged before adding MMP2/MMP3 > complications into the mix. At that point, I will hopefully have time to > follow up developing MMP2/MMP3 support, which will involve the points > mentioned below. > > A hacky patch is also included below, which makes the driver run on this > platform. I'm prepared to do the heavy lifting in implementing these changes > properly, but any high level guidance would be appreciated, especially as I > am new to the world of graphics. > > Ordered roughly from highest to lowest importance: > > > 1. Device tree support [snip] > > 2. Panel support. [snip] > > 3. Register space conflicts [snip] > > 4. Video memory [snip] > > 5. Output paths [snip] > > I will work on getting you an XO in case you are interested in testing the > driver there from time to time or even helping to develop support. But first I > need to get it bootable on mainline kernels (patches posted, waiting for > review). > > Thanks! > Daniel Hi Daniel, Do you know that there are 2 drm drivers for the Cubox? I posted mine (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) before Russell, but I got no return about it yet. As it uses the CMA helper (no handling of the Cubox GPU/VPU), my driver is simpler and does not need any specific memory reservation. It has full DT support. The Cubox specific drivers are build as loadable modules (dove-drm driver, tda998x hdmi slave encoder, si5351 clock driver and kirkwood i2s/spdif audio driver). The synchronization of module loading at startup time is done by EPROBE_DEFER. The DT permits each CRTC to use any of up to 4 clocks. It is designed to handle 2 CTRCs and 2 couples of encoder/connector only. LCD panel description (modeline / dimension) is done in the DT. If you are interested or simply curious, I put my whole Cubox work in http://moinejf.free.fr/cubox/ (I have some fixes that I will upload as soon as I have a running 3.10.0-rc7 kernel compiled with gcc 4.8.1!). The big kernel patch contains the dove-drm driver (in drivers/gpu/drm/dove/) and the Cubox DT (arch/arm/boot/dts/dove.dtsi and dove-cubox.dts).
On Wed, Jun 26, 2013 at 06:42:50PM +0200, Jean-Francois Moine wrote: > Do you know that there are 2 drm drivers for the Cubox? I posted mine > (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) > before Russell, but I got no return about it yet. > > As it uses the CMA helper (no handling of the Cubox GPU/VPU), my > driver is simpler and does not need any specific memory reservation. As I have previously covered, that's a *big* negative point - the big downside of that is that it will be *much* slower when it comes to using the GPU because the pixmap data will be accessed by the CPU via uncacheable mappings. Moreover, as you say you're not using the GPU, that means that every X operation which uses a DRM allocated pixmap will be copying between one uncached pixmap and another uncached pixmap. That is totally insane. > It has full DT support. The Cubox specific drivers are build as > loadable modules (dove-drm driver, tda998x hdmi slave encoder, si5351 > clock driver and kirkwood i2s/spdif audio driver). The synchronization > of module loading at startup time is done by EPROBE_DEFER. The DT > permits each CRTC to use any of up to 4 clocks. Via a horrid hack that doesn't really justify being in the kernel, and certainly isn't flexible because it makes use of global variables to detect when all the different parts of the DT representation have been registered. I have *serious* concerns about your approach to that problem, and I really violently detest your "solution" - which is more of a hack than a real solution. I've covered those points in my comments on your code when you first published it.
On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote: > Hi Russell, > > Thanks a lot for writing the Armada DRM driver. > > I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 > aka PXA2128). After a bit of fighting, I have it running. Could you share your > X driver, or your methodology for testing hardware cursors? I'd like to test > your work there too. > > It's probably easiest to get your cubox driver merged before adding MMP2/MMP3 > complications into the mix. At that point, I will hopefully have time to > follow up developing MMP2/MMP3 support, which will involve the points > mentioned below. As a side note, you've looked at a previous generation of the patches - in that same thread, there are two versions of these patches. The original set was an older version that I mis-posted. The later version removes the clock handling for the LCDs out of armada_crtc.c into armada_drv.c.
On Wed, Jun 26, 2013 at 10:42 AM, Jean-Francois Moine <moinejf@free.fr> wrote: > Do you know that there are 2 drm drivers for the Cubox? I posted mine > (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) > before Russell, but I got no return about it yet. I thought there was some agreement that you would merge your efforts into Russell's driver. Is that still the plan? Thanks for the links - I think we can learn something about timer handling from your work. I'll send a patch shortly incorporating the basis of something like this into armada_drm. Daniel
On Fri, Jun 28, 2013 at 01:54:21PM -0600, Daniel Drake wrote: > On Wed, Jun 26, 2013 at 10:42 AM, Jean-Francois Moine <moinejf@free.fr> wrote: > > Do you know that there are 2 drm drivers for the Cubox? I posted mine > > (http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168732.html) > > before Russell, but I got no return about it yet. > > I thought there was some agreement that you would merge your efforts > into Russell's driver. Is that still the plan? > > Thanks for the links - I think we can learn something about timer > handling from your work. I'll send a patch shortly incorporating the > basis of something like this into armada_drm. Note that my driver has changed significantly since the last posting; it's now using drm planes and also dmabuf stuff to limited extents (because dmabuf is technically misdesigned for CPUs with noncoherent DMA.)
On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote: > I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 > aka PXA2128). After a bit of fighting, I have it running. Could you share your > X driver, or your methodology for testing hardware cursors? I'd like to test > your work there too. BTW... a point on this. As you don't have the LCD_SPU_ADV_REG register, you probably don't have support for ARGB cursors. DRM only supports ARGB cursors. You can't down-convert an ARGB cursor to something which looks reasonable in the kernel, so I'd suggest forgetting hardware cursors. Even converting ARGB to transparency + RGB looks rubbish and wrong.
On Fri, Jun 28, 2013 at 3:27 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote: >> I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 >> aka PXA2128). After a bit of fighting, I have it running. Could you share your >> X driver, or your methodology for testing hardware cursors? I'd like to test >> your work there too. > > BTW... a point on this. As you don't have the LCD_SPU_ADV_REG register, > you probably don't have support for ARGB cursors. DRM only supports ARGB > cursors. You can't down-convert an ARGB cursor to something which looks > reasonable in the kernel, so I'd suggest forgetting hardware cursors. > Even converting ARGB to transparency + RGB looks rubbish and wrong. Interesting. Yes, a previous developer battled unsuccessfully with hardware cursors and in the end we ended up using low color depth ones which don't look great. I was wondering if you had found something new, but it sounds like that we really are limited by the hardware. Thanks Daniel
On Fri, Jun 28, 2013 at 03:36:37PM -0600, Daniel Drake wrote: > On Fri, Jun 28, 2013 at 3:27 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Jun 25, 2013 at 04:47:26PM -0400, Daniel Drake wrote: > >> I have tested it on OLPC XO-1.75 (MMP2 aka Armada610) and OLPC XO-4 (MMP3 > >> aka PXA2128). After a bit of fighting, I have it running. Could you share your > >> X driver, or your methodology for testing hardware cursors? I'd like to test > >> your work there too. > > > > BTW... a point on this. As you don't have the LCD_SPU_ADV_REG register, > > you probably don't have support for ARGB cursors. DRM only supports ARGB > > cursors. You can't down-convert an ARGB cursor to something which looks > > reasonable in the kernel, so I'd suggest forgetting hardware cursors. > > Even converting ARGB to transparency + RGB looks rubbish and wrong. > > Interesting. Yes, a previous developer battled unsuccessfully with > hardware cursors and in the end we ended up using low color depth ones > which don't look great. I was wondering if you had found something > new, but it sounds like that we really are limited by the hardware. The "something new" is that the Armada 510 has support for ARGB, not quite in the size that X and DRM prefers (64x64), but nevertheless it's full alpha-blended RGB. 64x32 seems to work and X seems to be happy with it - but there's no way at the moment for DRM to tell X about that kind of capability (so a generic kms driver can't use it.) However, as I say, that's not available on your SoC if you don't have the LCD_SPU_ADV_REG register. My plan is to push the cursor support out to the growing variant backends, and leave it unimplemented on anything but Armada 510.
diff --git a/drivers/gpu/drm/armada/Makefile b/drivers/gpu/drm/armada/Makefile index 430f025..aff908f 100644 --- a/drivers/gpu/drm/armada/Makefile +++ b/drivers/gpu/drm/armada/Makefile @@ -1,6 +1,6 @@ armada-y := armada_crtc.o armada_drv.o armada_fb.o armada_fbdev.o \ armada_gem.o armada_output.o armada_overlay.o \ - armada_slave.o + armada_slave.o dumb_panel.o armada-$(CONFIG_DEBUG_FS) += armada_debugfs.o obj-$(CONFIG_DRM_ARMADA) := armada.o diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index e5ab4cb..85f4d34 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -493,8 +493,16 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc, armada_reg_queue_set(regs, i, dcrtc->v[0].spu_v_porch, LCD_SPU_V_PORCH); armada_reg_queue_set(regs, i, dcrtc->v[0].spu_v_h_total, LCD_SPUT_V_H_TOTAL); - armada_reg_queue_mod(regs, i, dcrtc->v[0].spu_adv_reg | ADV_VSYNCOFFEN, - (0xfff << 20 | 0xfff), LCD_SPU_ADV_REG); + // FIXME this reg doesnt correspond to MMP2/MMP3 + // on MMP2/MMP3 it is a TV path register and one of the bits set below + // causes LCD output to be sent to the TV - oops! + //armada_reg_queue_mod(regs, i, dcrtc->v[0].spu_adv_reg | ADV_VSYNCOFFEN, + // (0xfff << 20 | 0xfff), LCD_SPU_ADV_REG); + + // FIXME doesnt seem to be a requirement, but it would be a good idea + // to write to reg 13c here, LCD_PN_SEPXLCNT + // Panel VSYNC Pulse Pixel Edge Control Register + // like drivers/video/mmp val = dcrtc->cfg_dma_ctrl0; @@ -734,9 +742,16 @@ int armada_drm_crtc_create(struct drm_device *dev, unsigned num, dcrtc->base = base; dcrtc->num = num; dcrtc->clk = clk; - dcrtc->cfg_sclk = 0xc0000000; + + // on MMP3 bits 31:29 select the clock, OLPC wants 0x1 here, LCD clock 1 + // on MMP2 bits 31:30 select the clock, OLPC wants 0x1 here, LCD clock 1 + dcrtc->cfg_sclk = 0x20001100; // FIXME hardcoded mmp3 value dcrtc->cfg_dma_ctrl0 = CFG_GRA_ENA | CFG_GRA_HSMOOTH; - dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0; + + // OLPC panel is 18 bit RGB666 + // FIXME: should be selected by panel driver? + dcrtc->cfg_dumb_ctrl = DUMB18_RGB666_0; + spin_lock_init(&dcrtc->irq_lock); dcrtc->irq_ena = CLEAN_SPU_IRQ_ISR; INIT_LIST_HEAD(&dcrtc->vbl_list); @@ -752,7 +767,8 @@ int armada_drm_crtc_create(struct drm_device *dev, unsigned num, writel_relaxed(0x00000000, dcrtc->base + LCD_SPU_GRA_OVSA_HPXL_VLN); /* Lower the watermark so to eliminate jitter at higher bandwidths */ - armada_updatel(0x20, (1 << 11) | 0xff, dcrtc->base + LCD_CFG_RDREG4F); + // FIXME this reg seems totally different on MMP, its LCD_PN_SEPXLCNT Panel VSYNC Pulse Pixel Edge Control Register + //armada_updatel(0x20, (1 << 11) | 0xff, dcrtc->base + LCD_CFG_RDREG4F); /* Ensure AXI pipeline is enabled */ armada_updatel(CFG_ARBFAST_ENA, 0, dcrtc->base + LCD_SPU_DMA_CTRL0); diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h index c73e29b..0417231 100644 --- a/drivers/gpu/drm/armada/armada_drm.h +++ b/drivers/gpu/drm/armada/armada_drm.h @@ -7,6 +7,8 @@ */ #ifndef ARMADA_DRM_H #define ARMADA_DRM_H +#include <drm/drm_mm.h> +#include <drm/drmP.h> struct armada_crtc; struct armada_gem_object; @@ -56,6 +58,8 @@ int armada_overlay_create(struct drm_device *); void armada_overlay_destroy(struct drm_device *); void armada_drm_overlay_off(struct drm_device *, struct armada_overlay *); +struct drm_connector *armada_dumb_panel_create(struct drm_device *dev); + int armada_drm_debugfs_init(struct drm_minor *); void armada_drm_debugfs_cleanup(struct drm_minor *); diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index bb70cf5..03f59d5 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -72,7 +72,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) if (!res[n]) break; - clk = clk_get_sys("dovefb.0", "extclk"); + clk = clk_get(&dev->platformdev->dev, NULL); if (IS_ERR(clk)) { DRM_ERROR("failed to get clock\n"); ret = PTR_ERR(clk); @@ -88,6 +88,8 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) } } + armada_dumb_panel_create(dev); + ret = drm_vblank_init(dev, n); if (ret) goto err_kms; @@ -298,12 +300,19 @@ static int armada_drm_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id mmp_disp_of_match[] = { + { .compatible = "marvell,mmp-display", }, + {}, +}; +MODULE_DEVICE_TABLE(of, mmp_disp_of_match); + static struct platform_driver armada_drm_platform_driver = { .probe = armada_drm_probe, .remove = armada_drm_remove, .driver = { .owner = THIS_MODULE, .name = "armada-drm", + .of_match_table = mmp_disp_of_match, }, }; diff --git a/drivers/gpu/drm/armada/armada_output.h b/drivers/gpu/drm/armada/armada_output.h index d655655..7bc406a 100644 --- a/drivers/gpu/drm/armada/armada_output.h +++ b/drivers/gpu/drm/armada/armada_output.h @@ -7,6 +7,8 @@ */ #ifndef ARMADA_CONNETOR_H #define ARMADA_CONNETOR_H +#include <linux/types.h> +#include <drm/drm_crtc.h> #define encoder_helper_funcs(encoder) \ ((struct drm_encoder_helper_funcs *)encoder->helper_private) diff --git a/drivers/gpu/drm/armada/dumb_panel.c b/drivers/gpu/drm/armada/dumb_panel.c new file mode 100644 index 0000000..f100505 --- /dev/null +++ b/drivers/gpu/drm/armada/dumb_panel.c @@ -0,0 +1,163 @@ +/* + * Copyright (C) 2012 Russell King + * Rewritten from the dovefb driver, and Armada510 manuals. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include "armada_output.h" +#include "armada_drm.h" +#include <linux/platform_device.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_crtc.h> + +struct dove_drm_dumb_panel_encoder { + struct drm_encoder base; + struct drm_encoder_helper_funcs encoder_helpers; +}; +#define to_dumb_panel_encoder(enc) container_of(enc, struct dove_drm_dumb_panel_encoder, base) + +static int dove_drm_connector_dumb_panel_mode_valid(struct drm_connector *conn, + struct drm_display_mode *mode) +{ + return 0; +} + +static int dove_drm_connector_dumb_panel_get_modes(struct drm_connector *conn) +{ + struct drm_display_mode *mode; + struct drm_encoder *enc = conn->encoder; + + mode = drm_mode_create(conn->dev); + mode->type = DRM_MODE_TYPE_DRIVER; + mode->clock = 57143; + mode->vrefresh = 50; + mode->hdisplay = 1200; + mode->hsync_start = mode->hdisplay + 26; // add right margin + mode->hsync_end = mode->hsync_start + 6; // add hsync len + mode->htotal = mode->hsync_end + 24; // add left margin + mode->vdisplay = 900; + mode->vsync_start = mode->vdisplay + 4; // add lower margin + mode->vsync_end = mode->vsync_start + 3; // add vsync len + mode->vtotal = mode->vsync_end + 5; // add top margin + mode->flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC; + + drm_mode_set_name(mode); + drm_mode_probed_add(conn, mode); + return 1; +} + +static bool dove_drm_dumb_panel_enc_mode_fixup(struct drm_encoder *encoder, + const struct drm_display_mode *mode, struct drm_display_mode *adjusted) +{ + return true; +} + +static void dove_drm_dumb_panel_enc_destroy(struct drm_encoder *enc) +{ + struct dove_drm_dumb_panel_encoder *tenc = to_dumb_panel_encoder(enc); + + drm_encoder_cleanup(&tenc->base); + kfree(tenc); +} + +static const struct drm_encoder_funcs dove_drm_dumb_panel_enc_funcs = { + .destroy = dove_drm_dumb_panel_enc_destroy, +}; + +static const struct drm_connector_helper_funcs dove_drm_conn_dumb_panel_helper_funcs = { + .get_modes = dove_drm_connector_dumb_panel_get_modes, + .mode_valid = dove_drm_connector_dumb_panel_mode_valid, + .best_encoder = armada_drm_connector_encoder, +}; + +static enum drm_connector_status dove_drm_conn_dumb_panel_detect( + struct drm_connector *conn, bool force) +{ + return connector_status_connected; +} + +static void panel_encoder_dpms(struct drm_encoder *encoder, int mode) +{ +} + +static void panel_encoder_mode_set(struct drm_encoder *encoder, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ +} + +static int dove_drm_conn_dumb_panel_create(struct drm_connector *conn, + struct drm_encoder **enc_ret) +{ + struct dove_drm_dumb_panel_encoder *tenc; + int ret; + drm_connector_helper_add(conn, &dove_drm_conn_dumb_panel_helper_funcs); + + tenc = kzalloc(sizeof(*tenc), GFP_KERNEL); + if (!tenc) + return -ENOMEM; + + tenc->base.possible_crtcs = 1 << 0; + ret = drm_encoder_init(conn->dev, &tenc->base, + &dove_drm_dumb_panel_enc_funcs, + DRM_MODE_ENCODER_DAC); // FIXME DAC correct? + if (ret) { + DRM_ERROR("unable to init encoder\n"); + kfree(tenc); + return ret; + } + tenc->encoder_helpers.dpms = panel_encoder_dpms; + tenc->encoder_helpers.mode_fixup = dove_drm_dumb_panel_enc_mode_fixup; + tenc->encoder_helpers.prepare = armada_drm_encoder_prepare; + tenc->encoder_helpers.commit = armada_drm_encoder_commit; + tenc->encoder_helpers.mode_set = panel_encoder_mode_set; + drm_encoder_helper_add(&tenc->base, &tenc->encoder_helpers); + + conn->encoder = &tenc->base; + if (enc_ret) + *enc_ret = &tenc->base; + + return ret; +} + +static int dove_drm_conn_dumb_panel_set_property(struct drm_connector *conn, + struct drm_property *property, uint64_t value) +{ + return 0; +} + +static const struct armada_output_type armada_drm_conn_dumb_panel = { + .connector_type = DRM_MODE_CONNECTOR_VGA, // FIXME correct? + .detect = dove_drm_conn_dumb_panel_detect, + .create = dove_drm_conn_dumb_panel_create, + .set_property = dove_drm_conn_dumb_panel_set_property, +}; + +struct drm_connector *armada_dumb_panel_create(struct drm_device *dev) +{ + return armada_output_create(dev, &armada_drm_conn_dumb_panel, NULL); +} + +static int dumb_probe(struct platform_device *pdev) +{ + // FIXME... create panel instance from DT data + return 0; +} + +static const struct of_device_id dumb_of_match[] = { + { .compatible = "marvell,dumb-panel", }, + {}, +}; +MODULE_DEVICE_TABLE(of, dumb_of_match); + +static struct platform_driver dumb_driver = { + .driver = { + .name = "dumb-panel", + .owner = THIS_MODULE, + .of_match_table = dumb_of_match, + }, + .probe = dumb_probe, +}; +module_platform_driver(dumb_driver);