Re: [PATCH 2/3] drm modesetting core
On Friday, May 18, 2007 12:33 pm Luca Tettamanti wrote: > > Yeah, there's more sharing that could be done... though I don't > > think the fb layer has the bits to actually grab EDIDs. > > There are the I2C functions (fb_do_probe_ddc_edid, fb_ddc_read - I > wrote them for the radeon driver, but now are available for general > use) which will issue the read command; fbmon.c has the stuff for > parsing the EDID; you usualy build a DB of supported modes which is > then used to validate the mode requested by the user. Of course each > driver has to implement the I2C adapter. I'll take a look at fbmon... I've seen the fb_ddc_read stuff but didn't see many drivers using it heavily. I think it makes sense to reuse your code where possible (in fact some earlier versions of the code made more use of FB stuff but was removed or rewritten for various reasons). > > Also, DRM is shared with BSD... > > Your patch already uses 'struct i2c_adapter' in drm_edid.c, is it > portable? I'm not sure how portable that will be. But regardless, if Linux has some of this code already, I'd like to reuse it. I'll go head and see what I can rip out. In fact, I've received some comments pushing me towards moving the core code (crtc, mode management) to drivers/video instead of DRM. That might make sense, especially if we can just use/extend the FB layer's mode tracking structures. Thanks, Jesse - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drm modesetting core
Il Thu, May 17, 2007 at 06:04:54PM -0700, Jesse Barnes ha scritto: > 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 000..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. There are the I2C functions (fb_do_probe_ddc_edid, fb_ddc_read - I wrote them for the radeon driver, but now are available for general use) which will issue the read command; fbmon.c has the stuff for parsing the EDID; you usualy build a DB of supported modes which is then used to validate the mode requested by the user. Of course each driver has to implement the I2C adapter. > Also, DRM is shared with BSD... Your patch already uses 'struct i2c_adapter' in drm_edid.c, is it portable? Luca -- "Vorrei morire ucciso dagli agi. Vorrei che di me si dicesse: ``Com'è morto?'' ``Gli è scoppiato il portafogli''" -- Marcello Marchesi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drm modesetting core
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 000..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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drm modesetting core
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 000..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.
> + * Copyright (C) 2006 Dennis Munsie <[EMAIL PROTECTED]>
> + */
> +#include
> +#include
> +#include "drmP.h"
> +#include "drm_edid.h"
> +
> +/* Valid EDID header has these bytes */
> +static u8 edid_header[] = { 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> 0x00 };
> +
> +/**
> + * edid_valid - sanity check EDID data
> + * @edid: EDID data
> + *
> + * Sanity check the EDID block by looking at the header, the version
> number
> + * and the checksum. Return 0 if the EDID doesn't check out, or 1 if
> it's
> + * valid.
> + */
> +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.
> +
> +/**
> + * drm_mode_std - convert standard mode info (width, height, refresh)
> into mode
> + * @t: standard timing params
> + *
> + * Take the standard timing params (in this case width, aspect, and
> refresh)
> + * and convert them into a real mode using CVT.
> + *
> + * Punts for now, but should eventually use the FB layer's CVT based
> mode
> + * generation code.
> + */
> +struct drm_display_mode *drm_mode_std(struct drm_device *dev,
> + struct std_timing *t)
> +{
get_std_timing?
> +/**
> + * drm_mode_detailed - create a new mode from an EDID detailed timing
> section
> + * @timing: EDID detailed timing info
> + * @preferred: is this a preferred mode?
> + *
> + * An EDID detailed timing block contains enough info for us to create
> and
> + * return a new struct drm_display_mode. The @preferred flag will be
> set
> + * if this is the display's preferred timing, and we'll use it to
> indicate
> + * to the other layers that this mode is desired.
> + */
> +struct drm_display_mode *drm_mode_detailed(drm_device_t *dev,
> +struct detailed_timing *timing)
> +{
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.
> +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.
> diff --git a/linux-core/drm_fb.c b/linux-core/drm_fb.c
> new file mode 100644
> index 000..ef05341
> --- /dev/null
> +++ b/linux-core/drm_fb.c
> @@ -0,0 +1,201 @@
> +/*
> + * Copyright © 2007 David Airlie
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files
> (the "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions
> of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * David Airlie
> + */
> +/*
> + * Modularization
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
Re: [PATCH 2/3] drm modesetting core
On Thursday, May 17, 2007 3:37 pm Jesse Barnes wrote: > This patch adds the core of the new DRM based modesetting system. It > creates several new structures in the DRM, the primary ones being the > CRTC, which controls all aspects of your device's CRTC(s), output, > which describes and controls the various outputs on your gfx chip > (e.g. TMDS, LVDS, VGA, etc.), and mode, which describes graphics > modes in enough detail for the output and CRTC callbacks to program > them to hardware. > > It also contains the user level IOCTL interfaces for doing graphics > programming (getting CRTC, output and mode lists, setting up new > frame buffer objects, etc.). > > It relies on the new TTM patch Dave posted recently. Makefile.kernel |6 drmP.h | 11 drm_bo.c|8 drm_bo_move.c |2 drm_bufs.c |3 drm_compat.h|7 drm_crtc.c | 1652 drm_crtc.h | 535 ++ drm_drv.c | 92 --- drm_edid.c | 467 +++ drm_edid.h | 176 + drm_fb.c| 201 ++ drm_fops.c |4 drm_modes.c | 558 ++ drm_objects.h | 22 drm_os_linux.h | 18 drm_stub.c | 21 drm_vm.c|5 18 files changed, 3695 insertions(+), 93 deletions(-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
