| Message ID | 1302892813-4529-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/15/11 2:40 PM, Jesse Barnes wrote: > @@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid, > info->bpp = 0; > break; > } > + > + if (edid->features & DRM_EDID_FEATURE_RGB) > + info->color_formats = DRM_COLOR_FORMAT_RGB444; > + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444) > + info->color_formats = DRM_COLOR_FORMAT_RGB444 | > + DRM_COLOR_FORMAT_YCRCB444; > + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB) > + info->color_formats = DRM_COLOR_FORMAT_RGB444 | > + DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422; Overcomplicated (RGB is numerically 0, YCRCB is just two other values or'd together) and wrong (doesn't handle YCbCr 4:2:2 alone). You want: info->color_formats = DRM_COLOR_FORMAT_RGB444; if (edid->features & DRM_EDID_FEATURE_YCRCB444) info->color_formats |= DRM_COLOR_FORMAT_YCBCR444; if (edid->features & DRM_EDID_FEATURE_YCRCB422) info->color_formats |= DRM_COLOR_FORMAT_YCBCR422; ... which is corrected to not include RGB uselessly in the DRM_EDID_FEATURE_* tokens. I should have noticed that in your first patch, whoops. - ajax
On Fri, 15 Apr 2011 15:13:02 -0400 Adam Jackson <ajax@redhat.com> wrote: > On 4/15/11 2:40 PM, Jesse Barnes wrote: > > > @@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid, > > info->bpp = 0; > > break; > > } > > + > > + if (edid->features & DRM_EDID_FEATURE_RGB) > > + info->color_formats = DRM_COLOR_FORMAT_RGB444; > > + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444) > > + info->color_formats = DRM_COLOR_FORMAT_RGB444 | > > + DRM_COLOR_FORMAT_YCRCB444; > > + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB) > > + info->color_formats = DRM_COLOR_FORMAT_RGB444 | > > + DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422; > > Overcomplicated (RGB is numerically 0, YCRCB is just two other values > or'd together) and wrong (doesn't handle YCbCr 4:2:2 alone). You want: Arg, of course I have to mask out the value first, I'll fix that (my current test display conveniently sets RGB_YCRCB so I missed this in testing). > > info->color_formats = DRM_COLOR_FORMAT_RGB444; > if (edid->features & DRM_EDID_FEATURE_YCRCB444) > info->color_formats |= DRM_COLOR_FORMAT_YCBCR444; > if (edid->features & DRM_EDID_FEATURE_YCRCB422) > info->color_formats |= DRM_COLOR_FORMAT_YCBCR422; > > ... which is corrected to not include RGB uselessly in the > DRM_EDID_FEATURE_* tokens. I should have noticed that in your first > patch, whoops. I don't think EDID supports that? The docs I have here imply that either RGB, RGB + YCrCb444 or RGB + YCrCb444 + YCrCb422 are the only things we can report. Or is there a CEA block extension that allows for more granularity?
On Fri, 15 Apr 2011 12:19:31 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Fri, 15 Apr 2011 15:13:02 -0400 > Adam Jackson <ajax@redhat.com> wrote: > > info->color_formats = DRM_COLOR_FORMAT_RGB444; > > if (edid->features & DRM_EDID_FEATURE_YCRCB444) > > info->color_formats |= DRM_COLOR_FORMAT_YCBCR444; > > if (edid->features & DRM_EDID_FEATURE_YCRCB422) > > info->color_formats |= DRM_COLOR_FORMAT_YCBCR422; > > > > ... which is corrected to not include RGB uselessly in the > > DRM_EDID_FEATURE_* tokens. I should have noticed that in your first > > patch, whoops. > > I don't think EDID supports that? The docs I have here imply that > either RGB, RGB + YCrCb444 or RGB + YCrCb444 + YCrCb422 are the only > things we can report. > > Or is there a CEA block extension that allows for more granularity? Nevermind, I even had the define correct, I just missed it when adding the conditionals. Will fix things to look like the above as it's nicer than doing the switch. Thanks,
On 4/15/11 3:19 PM, Jesse Barnes wrote:
> Or is there a CEA block extension that allows for more granularity?
CEA has bits for the two YCbCr formats too, which we should also parse
since there's plenty of 1.3+CEA blocks in the world thanks to HDMI. For
CEA blocks version 2 and up (version number in byte 2 of the CEA block,
zero-based indexing), (1 << 5) of byte 3 is 4:4:4 and (1 << 4) is 4:2:2.
Same byte as what we already check for EDID_BASIC_AUDIO, if that's any
clearer. CEA spec contains the same language about always supporting
RGB though.
I don't have a good answer for what to do if you have a 1.4+CEA block
and the two bitfields are inconsistent, besides violence against the
monitor vendor.
- ajax
On Fri, 15 Apr 2011 15:36:22 -0400 Adam Jackson <ajax@redhat.com> wrote: > On 4/15/11 3:19 PM, Jesse Barnes wrote: > > > Or is there a CEA block extension that allows for more granularity? > > CEA has bits for the two YCbCr formats too, which we should also parse > since there's plenty of 1.3+CEA blocks in the world thanks to HDMI. For > CEA blocks version 2 and up (version number in byte 2 of the CEA block, > zero-based indexing), (1 << 5) of byte 3 is 4:4:4 and (1 << 4) is 4:2:2. > Same byte as what we already check for EDID_BASIC_AUDIO, if that's any > clearer. CEA spec contains the same language about always supporting > RGB though. Ok, sounds good. I can add that a separate function that runs after we fill out display_info from the input & feature bits. > I don't have a good answer for what to do if you have a 1.4+CEA block > and the two bitfields are inconsistent, besides violence against the > monitor vendor. Hm I guess we'd take the CEA block instead as it's probably fresher at least.
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index fb5ebd9..64f8b7f 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1429,6 +1429,7 @@ static void drm_add_display_info(struct edid *edid, /* driver figures it out in this case */ info->bpp = 0; + info->color_formats = 0; /* Only defined for 1.4 with digital displays */ if (edid->revision < 4) @@ -1461,6 +1462,15 @@ static void drm_add_display_info(struct edid *edid, info->bpp = 0; break; } + + if (edid->features & DRM_EDID_FEATURE_RGB) + info->color_formats = DRM_COLOR_FORMAT_RGB444; + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444) + info->color_formats = DRM_COLOR_FORMAT_RGB444 | + DRM_COLOR_FORMAT_YCRCB444; + else if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB) + info->color_formats = DRM_COLOR_FORMAT_RGB444 | + DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422; } /** diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index aaec097..5cc7008 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -183,7 +183,9 @@ enum subpixel_order { SubPixelNone, }; - +#define DRM_COLOR_FORMAT_RGB444 (1<<0) +#define DRM_COLOR_FORMAT_YCRCB444 (1<<1) +#define DRM_COLOR_FORMAT_YCRCB422 (1<<2) /* * Describes a given display (e.g. CRT or flat panel) and its limitations. */ @@ -198,8 +200,10 @@ struct drm_display_info { unsigned int min_vfreq, max_vfreq; unsigned int min_hfreq, max_hfreq; unsigned int pixel_clock; + unsigned int bpp; enum subpixel_order subpixel_order; + unsigned long color_formats; char *raw_edid; /* if any */ };
EDID 1.4 digital displays report the color spaces they support in the features block. Add support for grabbing this data and stuffing it into the display_info struct for driver use. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/drm_edid.c | 10 ++++++++++ include/drm/drm_crtc.h | 6 +++++- 2 files changed, 15 insertions(+), 1 deletions(-)