[lkml]   [2011]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH] gma500: Intel GMA500 staging driver
On Tue, Feb 22, 2011 at 12:17:46PM +0000, Alan Cox wrote:
> This is an initial staging driver for the GMA500. It's been stripped out
> of the PVR drivers and crunched together from various bits of code and
> different kernels.
> Currently it's unaccelerated but still pretty snappy even compositing with
> the frame buffer X server.
> Lots of work is needed to rework the ttm and bo interfaces from being
> ripped out and then 2D acceleration wants putting back for framebuffer
> and somehow eventually via DRM.

There's a pretty significant amount of code duplication between the
modesetting part of this and i915. In an idea world they'd be abstracted
enough to avoid that, but I suspect that the risk of breaking i915
(which is something people actually care about) in favour of making the
gma500 code (which is something people care about in a very different
way) probably doesn't make it worth it right now.

> I'm not a DRM expert so if there is anyone with a GMA500 who actually knows
> something about DRI internals then help would be most welcome.

I managed to palm mine off on a 9-year old, but we'll see if we can find

TTM is outside my comfort level, so we'll need to wait for some feedback
from others on that. You may also want to Cc: dri-devel to get more eyes
on it.

> @@ -0,0 +1,167 @@
> +/*
> + * psb backlight using HAL
We're not using HAL so much these days.

> + /* return locally cached var instead of HW read (due to DPST etc.) */
> + return psb_brightness;

Ugh. We're really supposed to be doing readback from the hardware here,
in case the firmware's changed something behind our back.

> + value = (CoreClock * MHz) / BLC_PWM_FREQ_CALC_CONSTANT;

This hardware is spectacular.

> +struct mrst_panel_descriptor_v1{
> + uint32_t Panel_Port_Control; /* 1 dword, Register 0x61180 if LVDS */

Caps *and* underscores? needs a -Wtaste switch.

> + uint16_t Panel_MIPI_Display_Descriptor;
> + /*16 bits, Defined as follows: */
> + /* if MIPI, 0x0000 if LVDS */

"Defined as follows if MIPI, 0x000 if LVDS"?

> +struct gct_ioctl_arg{

This never seems to be used. I'd be kind of terrified if it ever needed
to be. In fact, are any of these vbt structures used at the moment?

> +int drm_psb_force_pipeb;


> +int drm_idle_check_interval = 5;


> +int gfxrtdelay = 2 * 1000;


> +MODULE_PARM_DESC(force_pipeb, "Forces PIPEB to become primary fb");


> +MODULE_PARM_DESC(ta_mem_size, "TA memory size in kiB");

Seems unused?

> +MODULE_PARM_DESC(ospm, "switch for ospm support");

What does OSPM translate as these days?

> +MODULE_PARM_DESC(rtpm, "Specifies Runtime PM delay for GFX");

Seems unused?

> +MODULE_PARM_DESC(hdmi_edid, "EDID info for HDMI monitor");

Thankfully unused.

> +static struct pci_device_id pciidlist[] = {
> + { 0x8086, 0x8108, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_PSB_8108 },
> + { 0x8086, 0x8109, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_PSB_8109 },

What hardware does this translate to at the moment? There's various bits
that suggest Moorestown support, but I'm not clear whether it's included
here or whether it's just the Poulsbo parts.

> +#if 1 /* FIXME remove it after PO */


> + DRM_ERROR("Gatt must be 256M aligned. This is a bug.\n");


> + DRM_INFO("Run drivers on Poulsbo platform!\n");


> + /**
> + * Init lid switch timer.
> + * NOTE: must do this after psb_intel_opregion_init
> + * and psb_backlight_init
> + */
> + if (dev_priv->lid_state)
> + psb_lid_timer_init(dev_priv);

Gak. Shouldn't there be an SCI on state change on opregion systems?

> +#define DRIVER_NAME "pvrsrvkm"
> +#define DRIVER_DESC "drm driver for the Intel GMA500"
> +#define DRIVER_AUTHOR "Intel Corporation"
> +#define OSPM_PROC_ENTRY "ospm"
> +#define RTPM_PROC_ENTRY "rtpm"


> +++ b/drivers/staging/gma500/psb_gfx.mod.c

I don't think you need this one :)

> +void psb_intel_destory_bios(struct drm_device *dev)

Oh if only...

> +/*
> + * Driver<->VBIOS interaction occurs through scratch bits in
> + * GR18 & SWF*.
> + */

Do you actually support this? I'm pretty sure it's all ignored in i915,
for better or worse.

> +#define BIT0 0x00000001
> +#define BIT1 0x00000002
> +#define BIT2 0x00000004
> +#define BIT3 0x00000008
> +#define BIT4 0x00000010
> +#define BIT5 0x00000020
> +#define BIT6 0x00000040
> +#define BIT7 0x00000080
> +#define BIT8 0x00000100
> +#define BIT9 0x00000200
> +#define BIT10 0x00000400
> +#define BIT11 0x00000800
> +#define BIT12 0x00001000
> +#define BIT13 0x00002000
> +#define BIT14 0x00004000
> +#define BIT15 0x00008000
> +#define BIT16 0x00010000
> +#define BIT17 0x00020000
> +#define BIT18 0x00040000
> +#define BIT19 0x00080000
> +#define BIT20 0x00100000
> +#define BIT21 0x00200000
> +#define BIT22 0x00400000
> +#define BIT23 0x00800000
> +#define BIT24 0x01000000
> +#define BIT25 0x02000000
> +#define BIT26 0x04000000
> +#define BIT27 0x08000000
> +#define BIT28 0x10000000
> +#define BIT29 0x20000000
> +#define BIT30 0x40000000
> +#define BIT31 0x80000000

You have *got* to be kidding.

> +/* ************************************************************************* *\
> +The display module performs a software reset.
> +Registers are written with their SW Reset default values.
> +\* ************************************************************************* */

I'd be kind of happier if we kept to one style of boxing per header...

> diff --git a/drivers/staging/gma500/psb_intel_opregion.c b/drivers/staging/gma500/psb_intel_opregion.c

I know I said that I wasn't convinced there was a benefit in trying to
use the existing 915 code, but our opregion implementation is
sufficiently best-effort right now that I think this should really be
merged with the existing one and that split out as a separate module. We
completely ignore opregion-based lid handling now, which may contribute
to the number of lid state bugs we have on intel.

> +++ b/drivers/staging/gma500/psb_intel_sdvo.c

Does PSB really support SDVO? I thought by that period the only thing
it'd be used for was plug-in SDVO cards, which doesn't seem so likely
with PSB.

> +/*
> + * It is used to enable VBLANK interrupt
> + */
> +int psb_enable_vblank(struct drm_device *dev, int pipe)

I was confused until I read the comment. Wait.

> diff --git a/drivers/staging/gma500/psb_powermgmt.c b/drivers/staging/gma500/psb_powermgmt.c

I have to say that the code in here scares me, but we'll see how it

> +#ifdef CONFIG_MDFD_GL3

Ugh. No way to determine this at runtime? Having to build this
per-device sounds like a recipe for sadness.

> diff --git a/drivers/staging/gma500/psb_pvr_glue.c b/drivers/staging/gma500/psb_pvr_glue.c

Do we need this at all without the PVR code?

> +void psb_lid_timer_init(struct drm_psb_private *dev_priv)
> +{
> + struct timer_list *lid_timer = &dev_priv->lid_timer;
> + unsigned long irq_flags;
> +
> + spin_lock_init(&dev_priv->lid_lock);
> + spin_lock_irqsave(&dev_priv->lid_lock, irq_flags);
> +
> + init_timer(lid_timer);
> +
> + lid_timer->data = (unsigned long)dev_priv;
> + lid_timer->function = psb_lid_timer_func;
> + lid_timer->expires = jiffies + PSB_LID_DELAY;

So we're waking up 10 times a second just to check the lid state?

> diff --git a/drivers/staging/gma500/psb_ttm_fence.c b/drivers/staging/gma500/psb_ttm_fence.c

This really looks like it's intended to be core TTM functionality, so it
should probably go past dri-devel.

This is definitely the best gma500 driver we've seen yet, but it seems
like there's two directions it can go. If the intention for now is to
provide a kernel-quality unaccelerated 2D driver then there's a *lot*
more code that can just be ripped out. If we want the acceleration to
work then there's an argument that we should be abstracting that into a
more generic SGX layer that other drivers can make use of. Does omapfb
have any acceleration? If so, is there anything we can build on there?

Other than that, I think there's a few bogosities here. The only thing
I'd really ask about in terms of whether it's ready to go in staging is
whether all of those ioctls need to be exposed at the moment. Beyond
that I'd like to see the opregion code unified, but for the most part
we're talking about the kind of cleanups that staging is meant for.

Matthew Garrett |

 \ /
  Last update: 2011-02-22 16:43    [W:0.071 / U:45.768 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site