Messages in this thread | | | From | Jesse Barnes <> | Subject | Re: [PATCH 2/3] drm modesetting core | Date | Thu, 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/
| |