On Wednesday, November 19, 2008 10:07 pm Eric Anholt wrote:
> On Wed, 2008-11-05 at 16:49 -0800, Jesse Barnes wrote:
> > Adds support for DRM based mode setting on Intel integrated graphics
> > chips.
>
> Might be nice to note what hardware is/isn't supported and tested here.
> For example, native HDMI is missing from the 2d driver it looks like,
> and we still need to write DP support.
Yeah, good idea. I'll add that info.
> > +#ifdef RANDR_12_INTERFACE
> > + /**
> > + * Callback when an output's property has changed.
> > + */
> > + bool (*set_property)(struct intel_dvo_device *dvo,
> > + struct drm_property *property, uint64_t val);
> > +#endif
>
> RANDR_12_INTERFACE?
Heh, fixed. We have all the property stuff defined now, but I don't think we
have any DVO specific properties to worry about yet.
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c index 1c87509..6986cc3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> >
> > +#if defined(CONFIG_DRM_I915_KMS)
> > +MODULE_DEVICE_TABLE(pci, pciidlist);
> > +#endif
> > +
>
> I'm assuming the #ifdef here is because we can't actually attach to the
> pci device unless we're doing modesetting and won't be fighting fbdev
> for it (that is, if we win we keep them out). But I don't think this
> achieves that on its own, and I don't see it referenced, so I'm just
> confused what it does.
I think it creates a table that the userland module utilities can look at to
see which drivers go with each device. If we add this table, userland will
pick up i915 by default when it sees a supported device and autoload the
driver.
> > @@ -146,6 +159,23 @@ static struct drm_driver driver = {
> > static int __init i915_init(void)
> > {
> > driver.num_ioctls = i915_max_ioctl;
> > +
> > + /* if enabled by default */
> > +#if defined(CONFIG_DRM_I915_KMS) && defined(CONFIG_X86)
> > + driver.driver_features |= DRIVER_MODESET;
> > + if (i915_modeset == 0)
> > + driver.driver_features &= ~DRIVER_MODESET;
> > +#endif
> > + if (i915_modeset == 1)
> > + driver.driver_features |= DRIVER_MODESET;
>
> So I'm trying to get this straight: We default to KMS disabled, unless
> you're on 32-bit and have selected the KMS option? What's the logic
> here?
The logic is obtuse... I think it *should* be:
#ifdef CONFIG_DRM_I915_KMS
if (i915_modeset != 0)
set DRIVER_MODESET flag
#endif
if (i915_modeset == 1)
set DRIVER_MODESET flag
> > - ret = i915_gem_object_pin(obj, args->alignment);
> > - if (ret != 0) {
> > - drm_gem_object_unreference(obj);
> > - mutex_unlock(&dev->struct_mutex);
> > - return ret;
> > + if (obj_priv->pin_filp != NULL && obj_priv->pin_filp != file_priv) {
> > + DRM_ERROR("Already pinned in i915_gem_pin_ioctl(): %d\n",
> > + args->handle);
> > + return -EINVAL;
>
> lock leak
Fixed.
>
> > @@ -2813,7 +2839,8 @@ i915_gem_init_hws(struct drm_device *dev)
> > int ret;
> >
> > /* If we need a physical address for the status page, it's already
> > - * initialized at driver load time.
> > + * initialized at driver load time, unless kernel modesetting is
> > + * active.
> > */
>
> wtf?
Hm, looks like lies. Fixed. Clearly we need the HWS page to be set up no
matter what in the KMS case, which we do. This comment was left over from
some different init code I guess.
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index ccd3e7d..2ea58ce 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>
> Could we dump this diff if we're just #if 0ing it for now? It shouldn't
> be hard to hook it back up, but it seems like throwing it out for the
> moment would be better.
Yeah, I'll drop it for now.
> Also, are things getting set so that kms sets the
> aware-of-pre/postmodeset bit so that we turn vblank off when not used?
Nope, this hadn't been updated since those changes. Fixed now though.
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > b/drivers/gpu/drm/i915/i915_suspend.c index 5ddc6e5..1662213 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -1,5 +1,7 @@
> > /*
> > *
> > + * Copyright 2003 Tungsten Graphics, Inc., Cedar Park, Texas.
> > + * All Rights Reserved.
> > * Copyright 2008 (c) Intel Corporation
> > * Jesse Barnes <[EMAIL PROTECTED]>
> > *
>
> This is kind of surprising in the middle of this commit. Perhaps it
> should be separated out to note what happened.
Yeah it should just go upstream. The code originally came from i830_driver.c
but when it was refactored the copyright wasn't preserved. I'll drop the
change from this patch.
>
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c
> > b/drivers/gpu/drm/i915/intel_crt.c new file mode 100644
> > index 0000000..2505d98
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> >
> > +static void intel_crt_save(struct drm_connector *connector)
> > +{
> > +
> > +}
> > +
> > +static void intel_crt_restore(struct drm_connector *connector)
> > +{
> > +
> > +}
>
> Are the save/restore paths actually used? It looks like no. If so,
> they should probably not get included in the initial commit. On the
> other hand, it seems like for DVO in particular having the save/restore
> next to the other code is a really good plan.
Yeah, dropped these. Suspend/resume can be done much more cleanly with the
mode setting bits in-tree, but that doesn't have to be part of the initial
commit.
>
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c new file mode 100644
> > index 0000000..09f6350
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >
> > +void
> > +intel_set_vblank(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_crtc *crtc;
> > + struct intel_crtc *intel_crtc;
> > + int vbl_pipe = 0;
> > +
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (crtc->enabled)
> > + vbl_pipe |= (1<<intel_crtc->pipe);
> > + }
> > +
> > + dev_priv->vblank_pipe = vbl_pipe;
> > +// i915_enable_interrupt(dev);
> > +}
>
> // comments
This code is unused anyway. Dropped.
> > + /* if we want to turn of the cursor ignore width and height */
>
> s/of/off/
Fixed.
> > + bo = drm_gem_object_lookup(dev, file_priv, handle);
> > + if (!bo)
> > + return -ENOENT;
> > +
> > + obj_priv = bo->driver_private;
> > +
> > + if (bo->size < width * height * 4) {
> > + DRM_ERROR("buffer is to small\n");
> > + return -ENOMEM;
> > + }
>
> Refcount leak
Fixed.
> > +static struct drm_framebuffer *
> > +intel_user_framebuffer_create(struct drm_device *dev,
> > + struct drm_file *filp,
> > + struct drm_mode_fb_cmd *mode_cmd)
> > +{
> > + struct drm_gem_object *obj;
> > +
> > + obj = drm_gem_object_lookup(dev, filp, mode_cmd->handle);
> > + if (!obj)
> > + return NULL;
> > +
> > + return intel_framebuffer_create(dev, mode_cmd, obj);
> > +}
>
> I always find a lookup without unreference a bit surprising. In this
> case, it was correct for intel_framebuffer_create success, but if it
> failed it would leak refcount.
Fixed.
> > +static int intel_insert_new_fb(struct drm_device *dev, struct drm_file
> > *file_priv, + struct drm_framebuffer *fb,
> > struct drm_mode_fb_cmd
> > *mode_cmd) +{
> > + struct intel_framebuffer *intel_fb;
> > + struct drm_gem_object *obj;
> > + struct drm_crtc *crtc;
> > +
> > + intel_fb = to_intel_framebuffer(fb);
> > +
> > + mutex_lock(&dev->struct_mutex);
> > + obj = drm_gem_object_lookup(dev, file_priv, mode_cmd->handle);
> > +
> > + if (!obj) {
> > + mutex_unlock(&dev->struct_mutex);
> > + return -EINVAL;
> > + }
> > +
> > + intel_fb->obj = obj;
> > + drm_gem_object_unreference(intel_fb->obj);
> > + drm_helper_mode_fill_fb_struct(fb, mode_cmd);
> > + mutex_unlock(&dev->struct_mutex);
> > +
>
> lookup is safe without struct_mutex, and cleans up this path a little.
Yep, fixed.
> > + intel_setup_outputs(dev);
> > +
> > + /* setup fbs */
> > + //drm_initial_config(dev, false);
>
> // comments
Dropped.
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h new file mode 100644
> > index 0000000..d4b2e03
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -0,0 +1,126 @@
> > +/*
> > + * Copyright (c) 2006 Dave Airlie <[EMAIL PROTECTED]>
> > + * Copyright (c) 2007 Intel Corporation
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> Copyright without license
Fixed.
> > + * Authors:
> > + * Eric Anholt <[EMAIL PROTECTED]>
> > + */
> > +/*
> > + * Copyright 2006 Dave Airlie <[EMAIL PROTECTED]>
> > + */
>
> Are these copyrights not under the license above? If so, what?
Assume so, fixed (Dave can correct if otherwise).
> > + case INTEL_DVO_CHIP_LVDS:
> > + // connector = DRM_MODE_CONNECTOR_LVDS;
> > + drm_connector_init(dev, connector,
> > &intel_dvo_connector_funcs,
> > + DRM_MODE_CONNECTOR_LVDS);
> > + encoder_type = DRM_MODE_ENCODER_LVDS;
> > + break;
> > + }
>
> // comments
Fixed (along with some >80 column stuff).
> > + */
> > + /*
> > + * Modularization
> > + */
>
> Unclear comment
Yeah dropped that.
> > +static int
> > +var_to_refresh(const struct fb_var_screeninfo *var)
> > +{
> > + int xtot = var->xres + var->left_margin + var->right_margin +
> > + var->hsync_len;
> > + int ytot = var->yres + var->upper_margin + var->lower_margin +
> > + var->vsync_len;
> > +
> > + return (1000000000 / var->pixclock * 1000 + 500) / xtot / ytot;
> > +}*/
>
> Does this have any reason to live?
Looks like it may be a holdover from intelfb.c. Dropped.
> > +static struct fb_ops intelfb_ops = {
> > + .owner = THIS_MODULE,
> > + //.fb_open = intelfb_open,
> > + //.fb_read = intelfb_read,
> > + //.fb_write = intelfb_write,
> > + //.fb_release = intelfb_release,
> > + //.fb_ioctl = intelfb_ioctl,
>
> // comments
Fixed.
>
> > + .fb_check_var = intelfb_check_var,
> > + .fb_set_par = intelfb_set_par,
> > + .fb_setcolreg = intelfb_setcolreg,
> > + .fb_fillrect = cfb_fillrect,
> > + .fb_copyarea = cfb_copyarea, //intelfb_copyarea,
> > + .fb_imageblit = cfb_imageblit, //intelfb_imageblit,
> > + .fb_pan_display = intelfb_pan_display,
> > + .fb_blank = intelfb_blank,
> > +};
> >
> > +MODULE_LICENSE("GPL");
>
> Should this be "GPL and additional rights", or is the license header at
> the top of the file wrong?
Looks like it. I've added "additional rights" but Dave can change it if he
wants.
> > +/*
> > + * Copyright (c) 2006 Dave Airlie <[EMAIL PROTECTED]>
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> Are these copyrights not under the license above? If so, what?
Merged.
>
> > diff --git a/drivers/gpu/drm/i915/intel_modes.c
> > b/drivers/gpu/drm/i915/intel_modes.c new file mode 100644
> > index 0000000..79be357
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_modes.c
> > @@ -0,0 +1,61 @@
> > +/*
> > + * Copyright (c) 2007 Dave Airlie <[EMAIL PROTECTED]>
> > + * Copyright (c) 2007 Intel Corporation
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> License?
Fixed (same X license as elsewhere).
> > + * Authors:
> > + * Eric Anholt <[EMAIL PROTECTED]>
> > + */
> > +/*
> > + * Copyright 2006 Dave Airlie <[EMAIL PROTECTED]>
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> Are these copyrights not under the license above? If so, what?
Merged back into the top.
> > + if ((ret = i2c_transfer(&sdvo_priv->i2c_bus->adapter, msgs, 2)) == 2)
> > + {
> > +// DRM_DEBUG("got back from addr %02X = %02x\n", out_buf[0],
> > buf[0]);
>
> // comments
Merged all this into separate debug code. Makes the drm debug option much
more usable.
> > + /* Restore interrupt config */
> > + I915_WRITE(PIPEASTAT, pipeastat_save | PIPE_HOTPLUG_TV_INTERRUPT_STATUS
> > | + PIPE_HOTPLUG_INTERRUPT_STATUS);
> > +
> > + return type;
> > +}
>
> PIPESTAT whacking without being protected by the lock. This should
> probably go directly in i915_irq.c (would we ever not be interested in
> the hotplug interrupts, or be concerned about the rate they come in in
> that case?), or otherwise use the pipestat update function that we've
> got now.
Looks like it's best to use those functions at this point. Just have to make
sure no other code bangs on the TV bits.
--
Jesse Barnes, Intel Open Source Technology Center
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
