![]() | |||||||||||||
Messages in this thread |
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 | |||||||||||||