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 <[EMAIL PROTECTED]>
> > + *
> > + * 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
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
