Re: [PATCH 2/3] drm modesetting core

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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