lkml.org 
[lkml]   [2007]   [May]   [17]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
FromJesse Barnes <>
SubjectRe: [PATCH 2/3] drm modesetting core
DateThu, 17 May 2007 18:04:54 -0700
On Thursday, May 17, 2007, Luca Tettamanti wrote:
> Il Thu, May 17, 2007 at 03:37:45PM -0700, Jesse Barnes ha scritto:
> > This patch adds the core of the new DRM based modesetting system.>> A couple of comments on drm_fb since I'm somewhat familiar with fb code:
> > new file mode 100644> > index 0000000..0d06792> > --- /dev/null> > +++ b/linux-core/drm_edid.c> > @@ -0,0 +1,467 @@> > +/*> > + * Copyright (c) 2007 Intel Corporation> > + *   Jesse Barnes <jesse.barnes@intel.com>> > + *> > + * DDC probing routines (drm_ddc_read & drm_do_probe_ddc_edid)
> > originally from
> > + * FB layer.>> Hum, why are you duplicating them here? fbmon.c has the
> infrastructure for parsing and even fixing known-broken EDIDs.

Yeah, there's more sharing that could be done... though I don't think the 
fb layer has the bits to actually grab EDIDs.  Also, DRM is shared with 
BSD...

> > +static bool edid_valid(struct edid *edid)> > +{> > +	int i;> > +	u8 csum = 0;> > +	u8 *raw_edid = (u8 *)edid;> > +> > +	if (memcmp(edid->header, edid_header, sizeof(edid_header)))
> > +		goto bad;> > +	if (edid->version != 1)> > +		goto bad;> > +	if (edid->revision <= 0 || edid->revision > 3)
> > +		goto bad;
> > +
> > +	for (i = 0; i < EDID_LENGTH; i++)
> > +		csum += raw_edid[i];
> > +	if (csum)
> > +		goto bad;
> > +
> > +	return 1;
> > +
> > +bad:
> > +	return 0;
> > +}>> This is basically edid_check_header + edid_checksum.

Yep, pretty trivial stuff.

> get_detailed_timing?>> If you  can't use 'struct fb_videomode' we may refactor code around a
> common data structure instead of a copy&paste.

I agree that would be better.  I'll see what I can do to unify the two.

> > +static unsigned char *drm_do_probe_ddc_edid(struct i2c_adapter
> > *adapter)>> [...]>> > +static unsigned char *drm_ddc_read(struct i2c_adapter *adapter)>> [...]>> Copy and paste from fb_dcc.c; furthermore a fix in drm_ddc_read hasn't
> been backported to the original code.

I think the original tries a few times... but it's still buggy.  I've got 
an old EDID 1.1 monitor whose EDID block is fetched by X but not by this 
code (or the original FB code) so I think we still have some timing bugs 
to fix.

> > +	info = framebuffer_alloc(sizeof(struct drmfb_par), device);
> > +	if (!info){> > +		return -EINVAL;> > +	}>> -ENOMEM? Plus, spurious brackets.

Fixed, thanks.

> > +	if (register_framebuffer(info) < 0)
> > +		return -EINVAL;>> You leak the fb_info structure on error path.

Oops, I'll fix that too.

At this point though, the drm_fb driver isn't actually used.  I recently 
added the intel_fb driver (mostly using code from intelfb) so we could 
have an accelerated DRM FB driver, hopefully that one's ok.

Thanks for looking at it.

Jesse

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-05-18 03:07    [from the cache]
©2003-2008