On Wednesday, November 19, 2008 6:45 pm Eric Anholt wrote:
> On Fri, 2008-11-07 at 15:31 -0800, Jesse Barnes wrote:
> > [Justify this massive code drop here.]
>
> Don't forget this bit :)
Heh, will fix. Thanks a ton for looking through this; I know it's huge and in
some places hard to follow.
> > +static int drm_mode_object_get(struct drm_device *dev,
> > + struct drm_mode_object *obj, uint32_t obj_type)
> > +{
>
> Disagreement between function name and doc. Might want to assert the
> lock.
Fixed, including assertion (added a WARN(!mutex_is_locked...)).
> > +struct drm_display_mode *drm_mode_create(struct drm_device *dev)
> > +{
> > + struct drm_display_mode *nmode;
> > +
> > + nmode = kzalloc(sizeof(struct drm_display_mode), GFP_KERNEL);
> > + if (!nmode)
> > + return NULL;
> > +
> > + drm_mode_object_get(dev, &nmode->base, DRM_MODE_OBJECT_MODE);
>
> doc claims no locking but calling drm_mode_object_get with lock unheld.
Fixed, indicated that the mode config lock must be held.
> > + /* add enum element 0-2 */
> > + for (i = 0; i < 3; i++)
> > +
> > drm_property_add_enum(dev->mode_config.dvi_i_subconnector_property, i,
> > drm_subconnector_enum_list[i].type,
> > + drm_subconnector_enum_list[i].name);
>
> Hardcoded array indices so far from the definitions of the array are a
> little scary. Perhaps separate arrays for dvi-i/tv, and ARRAY_SIZE()?
Yeah, this is a little scary, as are the 150 character lines. I split the
arrays and added a "get name" function macro but Dave may have better ideas
about this.
>
> > +/**
> > + * drm_crtc_convert_to_umode - convert a modeinfo into a
> > drm_display_mode + * @out: drm_display_mode to return to the user
> > + * @in: drm_mode_modeinfo to use
> > + *
> > + * LOCKING:
> > + * None.
> > + *
> > + * Convert a drmo_mode_modeinfo into a drm_display_mode structure to
> > return to + * the caller.
>
> typo: drmo_mode_modeinfo
Fixed.
> > + /* handle this in 4 parts */
> > + /* FBs */
> > + if (card_res->count_fbs >= fb_count) {
> > + copied = 0;
> > + fb_id = (uint32_t *)(unsigned long)card_res->fb_id_ptr;
>
> (uint32_t __user *) to appease sparse
Fixed.
> > + if (req->flags & DRM_MODE_CURSOR_BO) {
> > + /* Turn of the cursor if handle is 0 */
>
> s/of/off/
Fixed.
>
> > + if (crtc->funcs->cursor_set) {
> > + ret = crtc->funcs->cursor_set(crtc, file_priv,
> > req->handle,
> > req->width, req->height); + } else {
> > + DRM_ERROR("crtc does not support cursor\n");
> > + ret = -EFAULT;
>
> these should be -EINVAL or -ENXIO or something not -EFAULT. (user
> didn't hand you a bad pointer)
Right. -ENXIO sounds good (-ENODEV would probably also work).
> > + goto out;
> > + }
> > + }
> > +
> > + if (req->flags & DRM_MODE_CURSOR_MOVE) {
> > + if (crtc->funcs->cursor_move) {
> > + ret = crtc->funcs->cursor_move(crtc, req->x, req->y);
> > + } else {
> > + DRM_ERROR("crtc does not support cursor\n");
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > + }
> > +out:
> > + mutex_unlock(&dev->mode_config.mutex);
> > + return ret;
> > +}
>
> This ioctl seems to be a 1:1 mapping of cursors to crtcs. As far as I
> understand, we have a many:1 mapping of cursors to crtcs possible on
> Intel, and it seems relevant for MPX. I guess that could be a future
> ioctl, though.
Yeah, I think multiple cursors will either have to be a new ioctl or just done
in higher level sw.
> > +/*
> > + *
> > + */
>
> /* Least informative comment ever. */
Oh yeah I like your comment better, fixed (added a real comment for the fn).
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> > b/drivers/gpu/drm/drm_crtc_helper.c new file mode 100644
> > index 0000000..a0f3ffe
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >
> > + /* XXX free adjustedmode */
> > + drm_mode_destroy(dev, adjusted_mode);
> > + /* TODO */
> > +// if (scrn->pScreen)
> > +// drm_crtc_set_screen_sub_pixel_order(dev);
>
> // comments
Woo dead code. Fixed all these up.
> > + */
> > +/*
> > + * Copyright © 2007 Dave Airlie
> > + */
>
> Is there a license associated with this copyright?
The file is licensed, so I'm not sure if it matters that the copyright is in
separate comment (after all, what does a comment license? the comment itself?
the next N lines?). But it's certainly cleaner to keep things together, so I
moved it to the top.
> > +/* FIXME: what we don't have a list sort function? */
> > +/* list sort from Mark J Roberts ([EMAIL PROTECTED]) */
>
> From what I could see, this would be GPL-licensed, but the header on
> this file says XF86.
Added a note at the top indicating that portions are GPL; not sure if I can
remove the XF86 license and just point at "COPYING" or not though...
> > --- /dev/null
> > +++ b/include/drm/drm_crtc.h
> > @@ -0,0 +1,716 @@
> > +/*
> > + * Copyright © 2006 Keith Packard
> > + * Copyright © 2007 Intel Corporation
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> Copyright with no license?
Fixed. Came from X so I added the X boilerplate.
>
> > diff --git a/include/drm/drm_crtc_helper.h
> > b/include/drm/drm_crtc_helper.h new file mode 100644
> > index 0000000..e770205
> > --- /dev/null
> > +++ b/include/drm/drm_crtc_helper.h
> > @@ -0,0 +1,96 @@
> > +/*
> > + * Copyright © 2006 Keith Packard
> > + * Copyright © 2007 Intel Corporation
> > + * Jesse Barnes <[EMAIL PROTECTED]>
> > + */
>
> License?
Ditto.
>
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > new file mode 100644
> > index 0000000..dccfba9
> > --- /dev/null
> > +++ b/include/drm/drm_edid.h
> > @@ -0,0 +1,178 @@
> > +#ifndef __DRM_EDID_H__
> > +#define __DRM_EDID_H__
> > +
> > +#include <linux/types.h>
>
> Copyright?
This one is all new, but I put it under the X license for consistency.
--
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
