Hi,
The Allwinner display engine doesn't have any kind of hardware help to deal with TV overscan.
This means that if we use the only mode the hardware provides (either PAL or NTSC, or something else), most of the screens will crop the borders of the image, which is bad.
We can however use somekind of a hack, to instead reduce the mode exposed to the userspace, and center it in the final image. We would expose different overscan ratio to be able to deal with most of the screens, each reducing more the displayable area.
The first patches are rework for the command line parser in order to be able to use named modes. This is going to be helpful for us, since different modes might have the same timings but only differ on a few settings not exposed in the modes, but it might eventually be used for the *VGA modes for example.
The last patches extend the current driver to deal with the changes used to introduce the overscan.
Let me know what you think, Maxime
Maxime Ripard (5): drm/modes: Rewrite the command line parser drm/modes: Support modes names on the command line drm/sun4i: Add custom crtc state drm/sun4i: Compute TCON1 mode from tv mode drm/sun4i: Add support for the overscan profiles
drivers/gpu/drm/drm_connector.c | 3 +- drivers/gpu/drm/drm_fb_helper.c | 4 +- drivers/gpu/drm/drm_modes.c | 324 +++++++++++++++++---------- drivers/gpu/drm/sun4i/sun4i_backend.c | 18 +- drivers/gpu/drm/sun4i/sun4i_crtc.c | 37 ++- drivers/gpu/drm/sun4i/sun4i_crtc.h | 16 +- drivers/gpu/drm/sun4i/sun4i_rgb.c | 10 +- drivers/gpu/drm/sun4i/sun4i_tv.c | 75 ++++-- include/drm/drm_connector.h | 1 +- 9 files changed, 342 insertions(+), 146 deletions(-)
Rewrite the command line parser in order to get away from the state machine parsing the video mode lines.
Hopefully, this will allow to extend it more easily to support named modes and / or properties set directly on the command line.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++-------------- 1 file changed, 190 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 53f07ac7c174..7d5bdca276f2 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -30,6 +30,7 @@ * authorization from the copyright holder(s) and author(s). */
+#include <linux/ctype.h> #include <linux/list.h> #include <linux/list_sort.h> #include <linux/export.h> @@ -1261,6 +1262,131 @@ void drm_mode_connector_list_update(struct drm_connector *connector) } EXPORT_SYMBOL(drm_mode_connector_list_update);
+static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr, + struct drm_cmdline_mode *mode) +{ + if (str[0] != '-') + return -EINVAL; + + mode->bpp = simple_strtol(str + 1, end_ptr, 10); + mode->bpp_specified = true; + + return 0; +} + +static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr, + struct drm_cmdline_mode *mode) +{ + if (str[0] != '@') + return -EINVAL; + + mode->refresh = simple_strtol(str + 1, end_ptr, 10); + mode->refresh_specified = true; + + return 0; +} + +static int drm_mode_parse_cmdline_extra(const char *str, int length, + struct drm_connector *connector, + struct drm_cmdline_mode *mode) +{ + int i; + + for (i = 0; i < length; i++) { + switch (str[i]) { + case 'i': + mode->interlace = true; + break; + case 'm': + mode->margins = true; + break; + case 'D': + if (mode->force != DRM_FORCE_UNSPECIFIED) + return -EINVAL; + + if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) && + (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB)) + mode->force = DRM_FORCE_ON; + else + mode->force = DRM_FORCE_ON_DIGITAL; + break; + case 'd': + if (mode->force != DRM_FORCE_UNSPECIFIED) + return -EINVAL; + + mode->force = DRM_FORCE_OFF; + break; + case 'e': + if (mode->force != DRM_FORCE_UNSPECIFIED) + return -EINVAL; + + mode->force = DRM_FORCE_ON; + break; + default: + return -EINVAL; + } + } + + return 0; +} + +static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length, + bool extras, + struct drm_connector *connector, + struct drm_cmdline_mode *mode) +{ + bool rb = false, cvt = false; + int xres = 0, yres = 0; + int remaining, i; + char *end_ptr; + + xres = simple_strtol(str, &end_ptr, 10); + + if (end_ptr[0] != 'x') + return -EINVAL; + end_ptr++; + + yres = simple_strtol(end_ptr, &end_ptr, 10); + + remaining = length - (end_ptr - str); + if (remaining < 0) + return -EINVAL; + + for (i = 0; i < remaining; i++) { + switch (end_ptr[i]) { + case 'M': + cvt = true; + break; + case 'R': + rb = true; + break; + default: + /* + * Try to pass that to our extras parsing + * function to handle the case where the + * extras are directly after the resolution + */ + if (extras) { + int ret = drm_mode_parse_cmdline_extra(end_ptr + i, + 1, + connector, + mode); + if (ret) + return ret; + } else { + return -EINVAL; + } + } + } + + mode->xres = xres; + mode->yres = yres; + mode->cvt = cvt; + mode->rb = rb; + + return 0; +} + /** * drm_mode_parse_command_line_for_connector - parse command line modeline for connector * @mode_option: optional per connector mode option @@ -1287,13 +1413,12 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, struct drm_cmdline_mode *mode) { const char *name; - unsigned int namelen; - bool res_specified = false, bpp_specified = false, refresh_specified = false; - unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0; - bool yres_specified = false, cvt = false, rb = false; - bool interlace = false, margins = false, was_digit = false; - int i; - enum drm_connector_force force = DRM_FORCE_UNSPECIFIED; + bool parse_extras = false; + unsigned int bpp_off = 0, refresh_off = 0; + unsigned int mode_end = 0; + char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; + char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; + int ret;
#ifdef CONFIG_FB if (!mode_option) @@ -1306,127 +1431,77 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, }
name = mode_option; - namelen = strlen(name); - for (i = namelen-1; i >= 0; i--) { - switch (name[i]) { - case '@': - if (!refresh_specified && !bpp_specified && - !yres_specified && !cvt && !rb && was_digit) { - refresh = simple_strtol(&name[i+1], NULL, 10); - refresh_specified = true; - was_digit = false; - } else - goto done; - break; - case '-': - if (!bpp_specified && !yres_specified && !cvt && - !rb && was_digit) { - bpp = simple_strtol(&name[i+1], NULL, 10); - bpp_specified = true; - was_digit = false; - } else - goto done; - break; - case 'x': - if (!yres_specified && was_digit) { - yres = simple_strtol(&name[i+1], NULL, 10); - yres_specified = true; - was_digit = false; - } else - goto done; - break; - case '0' ... '9': - was_digit = true; - break; - case 'M': - if (yres_specified || cvt || was_digit) - goto done; - cvt = true; - break; - case 'R': - if (yres_specified || cvt || rb || was_digit) - goto done; - rb = true; - break; - case 'm': - if (cvt || yres_specified || was_digit) - goto done; - margins = true; - break; - case 'i': - if (cvt || yres_specified || was_digit) - goto done; - interlace = true; - break; - case 'e': - if (yres_specified || bpp_specified || refresh_specified || - was_digit || (force != DRM_FORCE_UNSPECIFIED)) - goto done;
- force = DRM_FORCE_ON; - break; - case 'D': - if (yres_specified || bpp_specified || refresh_specified || - was_digit || (force != DRM_FORCE_UNSPECIFIED)) - goto done; + if (!isdigit(name[0])) + return false;
- if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) && - (connector->connector_type != DRM_MODE_CONNECTOR_HDMIB)) - force = DRM_FORCE_ON; - else - force = DRM_FORCE_ON_DIGITAL; - break; - case 'd': - if (yres_specified || bpp_specified || refresh_specified || - was_digit || (force != DRM_FORCE_UNSPECIFIED)) - goto done; + /* Try to locate the bpp and refresh specifiers, if any */ + bpp_ptr = strchr(name, '-'); + if (bpp_ptr) { + bpp_off = bpp_ptr - name; + mode->bpp_specified = true; + }
- force = DRM_FORCE_OFF; - break; - default: - goto done; - } + refresh_ptr = strchr(name, '@'); + if (refresh_ptr) { + refresh_off = refresh_ptr - name; + mode->refresh_specified = true; }
- if (i < 0 && yres_specified) { - char *ch; - xres = simple_strtol(name, &ch, 10); - if ((ch != NULL) && (*ch == 'x')) - res_specified = true; - else - i = ch - name; - } else if (!yres_specified && was_digit) { - /* catch mode that begins with digits but has no 'x' */ - i = 0; + /* Locate the end of the name / resolution, and parse it */ + if (bpp_ptr && refresh_ptr) { + mode_end = min(bpp_off, refresh_off); + } else if (bpp_ptr) { + mode_end = bpp_off; + } else if (refresh_ptr) { + mode_end = refresh_off; + } else { + mode_end = strlen(name); + parse_extras = true; } -done: - if (i >= 0) { - pr_warn("[drm] parse error at position %i in video mode '%s'\n", - i, name); - mode->specified = false; + + ret = drm_mode_parse_cmdline_res_mode(name, mode_end, + parse_extras, + connector, + mode); + if (ret) return false; - } + mode->specified = true;
- if (res_specified) { - mode->specified = true; - mode->xres = xres; - mode->yres = yres; + if (bpp_ptr) { + ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode); + if (ret) + return false; }
- if (refresh_specified) { - mode->refresh_specified = true; - mode->refresh = refresh; + if (refresh_ptr) { + ret = drm_mode_parse_cmdline_refresh(refresh_ptr, + &refresh_end_ptr, mode); + if (ret) + return false; }
- if (bpp_specified) { - mode->bpp_specified = true; - mode->bpp = bpp; + /* + * Locate the end of the bpp / refresh, and parse the extras + * if relevant + */ + if (bpp_ptr && refresh_ptr) + extra_ptr = max(bpp_end_ptr, refresh_end_ptr); + else if (bpp_ptr) + extra_ptr = bpp_end_ptr; + else if (refresh_ptr) + extra_ptr = refresh_end_ptr; + + if (extra_ptr) { + int remaining = strlen(name) - (extra_ptr - name); + + /* + * We still have characters to process, while + * we shouldn't have any + */ + if (remaining > 0) + return false; } - mode->rb = rb; - mode->cvt = cvt; - mode->interlace = interlace; - mode->margins = margins; - mode->force = force;
return true; }
On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Rewrite the command line parser in order to get away from the state machine parsing the video mode lines.
Hopefully, this will allow to extend it more easily to support named modes and / or properties set directly on the command line.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++-------------- 1 file changed, 190 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 53f07ac7c174..7d5bdca276f2 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -30,6 +30,7 @@
- authorization from the copyright holder(s) and author(s).
*/
+#include <linux/ctype.h> #include <linux/list.h> #include <linux/list_sort.h> #include <linux/export.h> @@ -1261,6 +1262,131 @@ void drm_mode_connector_list_update(struct drm_connector *connector) } EXPORT_SYMBOL(drm_mode_connector_list_update);
+static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
struct drm_cmdline_mode *mode)+{
if (str[0] != '-')return -EINVAL;mode->bpp = simple_strtol(str + 1, end_ptr, 10);mode->bpp_specified = true;return 0;+}
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
struct drm_cmdline_mode *mode)+{
if (str[0] != '@')return -EINVAL;mode->refresh = simple_strtol(str + 1, end_ptr, 10);mode->refresh_specified = true;return 0;+}
+static int drm_mode_parse_cmdline_extra(const char *str, int length,
struct drm_connector *connector,struct drm_cmdline_mode *mode)+{
int i;for (i = 0; i < length; i++) {switch (str[i]) {case 'i':mode->interlace = true;break;case 'm':mode->margins = true;break;case 'D':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&(connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))mode->force = DRM_FORCE_ON;elsemode->force = DRM_FORCE_ON_DIGITAL;break;case 'd':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;mode->force = DRM_FORCE_OFF;break;case 'e':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;mode->force = DRM_FORCE_ON;break;default:return -EINVAL;}}return 0;+}
+static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
bool extras,struct drm_connector *connector,struct drm_cmdline_mode *mode)+{
bool rb = false, cvt = false;int xres = 0, yres = 0;int remaining, i;char *end_ptr;xres = simple_strtol(str, &end_ptr, 10);
checkpatch is telling me to use kstrtol instead, as simple_strtol is deprecated
if (end_ptr[0] != 'x')
check that end_ptr != NULL? you should probably also check that xres isn't an error (ie: -ERANGE or -EINVAL)
return -EINVAL;end_ptr++;yres = simple_strtol(end_ptr, &end_ptr, 10);
check end_ptr != NULL and yres sane
remaining = length - (end_ptr - str);if (remaining < 0)
right, so if end_ptr is NULL here, we'll end up with a huge positive value for remaining :)
return -EINVAL;for (i = 0; i < remaining; i++) {switch (end_ptr[i]) {case 'M':cvt = true;
the previous code ensured proper ordering as well as parsing, whereas yours will take these in any order (and accepts duplicates). i don't think this should cause any issues, but perhaps something to verify.
break;case 'R':rb = true;break;default:/** Try to pass that to our extras parsing* function to handle the case where the* extras are directly after the resolution*/if (extras) {int ret = drm_mode_parse_cmdline_extra(end_ptr + i,1,connector,mode);if (ret)return ret;} else {return -EINVAL;}}}mode->xres = xres;mode->yres = yres;mode->cvt = cvt;mode->rb = rb;return 0;+}
/**
- drm_mode_parse_command_line_for_connector - parse command line modeline for connector
- @mode_option: optional per connector mode option
@@ -1287,13 +1413,12 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, struct drm_cmdline_mode *mode) { const char *name;
unsigned int namelen;bool res_specified = false, bpp_specified = false, refresh_specified = false;unsigned int xres = 0, yres = 0, bpp = 32, refresh = 0;bool yres_specified = false, cvt = false, rb = false;bool interlace = false, margins = false, was_digit = false;int i;enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
bool parse_extras = false;unsigned int bpp_off = 0, refresh_off = 0;unsigned int mode_end = 0;char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;int ret;#ifdef CONFIG_FB if (!mode_option) @@ -1306,127 +1431,77 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, }
name = mode_option;
namelen = strlen(name);for (i = namelen-1; i >= 0; i--) {switch (name[i]) {case '@':if (!refresh_specified && !bpp_specified &&!yres_specified && !cvt && !rb && was_digit) {refresh = simple_strtol(&name[i+1], NULL, 10);refresh_specified = true;was_digit = false;} elsegoto done;break;case '-':if (!bpp_specified && !yres_specified && !cvt &&!rb && was_digit) {bpp = simple_strtol(&name[i+1], NULL, 10);bpp_specified = true;was_digit = false;} elsegoto done;break;case 'x':if (!yres_specified && was_digit) {yres = simple_strtol(&name[i+1], NULL, 10);yres_specified = true;was_digit = false;} elsegoto done;break;case '0' ... '9':was_digit = true;break;case 'M':if (yres_specified || cvt || was_digit)goto done;cvt = true;break;case 'R':if (yres_specified || cvt || rb || was_digit)goto done;rb = true;break;case 'm':if (cvt || yres_specified || was_digit)goto done;margins = true;break;case 'i':if (cvt || yres_specified || was_digit)goto done;interlace = true;break;case 'e':if (yres_specified || bpp_specified || refresh_specified ||was_digit || (force != DRM_FORCE_UNSPECIFIED))goto done;force = DRM_FORCE_ON;break;case 'D':if (yres_specified || bpp_specified || refresh_specified ||was_digit || (force != DRM_FORCE_UNSPECIFIED))goto done;
if (!isdigit(name[0]))return false;
if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&(connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))force = DRM_FORCE_ON;elseforce = DRM_FORCE_ON_DIGITAL;break;case 'd':if (yres_specified || bpp_specified || refresh_specified ||was_digit || (force != DRM_FORCE_UNSPECIFIED))goto done;
/* Try to locate the bpp and refresh specifiers, if any */bpp_ptr = strchr(name, '-');if (bpp_ptr) {bpp_off = bpp_ptr - name;mode->bpp_specified = true;}
force = DRM_FORCE_OFF;break;default:goto done;}
refresh_ptr = strchr(name, '@');if (refresh_ptr) {refresh_off = refresh_ptr - name;mode->refresh_specified = true; }
if (i < 0 && yres_specified) {char *ch;xres = simple_strtol(name, &ch, 10);if ((ch != NULL) && (*ch == 'x'))res_specified = true;elsei = ch - name;} else if (!yres_specified && was_digit) {/* catch mode that begins with digits but has no 'x' */i = 0;
/* Locate the end of the name / resolution, and parse it */if (bpp_ptr && refresh_ptr) {mode_end = min(bpp_off, refresh_off);} else if (bpp_ptr) {mode_end = bpp_off;} else if (refresh_ptr) {mode_end = refresh_off;} else {mode_end = strlen(name);parse_extras = true; }-done:
if (i >= 0) {pr_warn("[drm] parse error at position %i in video mode '%s'\n",i, name);mode->specified = false;
ret = drm_mode_parse_cmdline_res_mode(name, mode_end,parse_extras,connector,mode);if (ret) return false;
}
mode->specified = true;
if (res_specified) {mode->specified = true;mode->xres = xres;mode->yres = yres;
if (bpp_ptr) {ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode);if (ret)return false; }
if (refresh_specified) {mode->refresh_specified = true;mode->refresh = refresh;
if (refresh_ptr) {ret = drm_mode_parse_cmdline_refresh(refresh_ptr,&refresh_end_ptr, mode);if (ret)return false; }
if (bpp_specified) {mode->bpp_specified = true;mode->bpp = bpp;
/** Locate the end of the bpp / refresh, and parse the extras* if relevant*/if (bpp_ptr && refresh_ptr)
Perhaps I'm paranoid, but I think it'd be better to check bpp_end_ptr && refresh_end_ptr in this conditional.
Sean
extra_ptr = max(bpp_end_ptr, refresh_end_ptr);else if (bpp_ptr)extra_ptr = bpp_end_ptr;else if (refresh_ptr)extra_ptr = refresh_end_ptr;if (extra_ptr) {int remaining = strlen(name) - (extra_ptr - name);/** We still have characters to process, while* we shouldn't have any*/if (remaining > 0)return false; }
mode->rb = rb;mode->cvt = cvt;mode->interlace = interlace;mode->margins = margins;mode->force = force; return true;}
git-series 0.8.10
Hi Sean,
Thanks for taking the time to review this.
On Wed, Nov 16, 2016 at 12:12:53PM -0500, Sean Paul wrote:
On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Rewrite the command line parser in order to get away from the state machine parsing the video mode lines.
Hopefully, this will allow to extend it more easily to support named modes and / or properties set directly on the command line.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/drm_modes.c | 305 +++++++++++++++++++++++-------------- 1 file changed, 190 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 53f07ac7c174..7d5bdca276f2 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -30,6 +30,7 @@
- authorization from the copyright holder(s) and author(s).
*/
+#include <linux/ctype.h> #include <linux/list.h> #include <linux/list_sort.h> #include <linux/export.h> @@ -1261,6 +1262,131 @@ void drm_mode_connector_list_update(struct drm_connector *connector) } EXPORT_SYMBOL(drm_mode_connector_list_update);
+static int drm_mode_parse_cmdline_bpp(const char *str, char **end_ptr,
struct drm_cmdline_mode *mode)+{
if (str[0] != '-')return -EINVAL;mode->bpp = simple_strtol(str + 1, end_ptr, 10);mode->bpp_specified = true;return 0;+}
+static int drm_mode_parse_cmdline_refresh(const char *str, char **end_ptr,
struct drm_cmdline_mode *mode)+{
if (str[0] != '@')return -EINVAL;mode->refresh = simple_strtol(str + 1, end_ptr, 10);mode->refresh_specified = true;return 0;+}
+static int drm_mode_parse_cmdline_extra(const char *str, int length,
struct drm_connector *connector,struct drm_cmdline_mode *mode)+{
int i;for (i = 0; i < length; i++) {switch (str[i]) {case 'i':mode->interlace = true;break;case 'm':mode->margins = true;break;case 'D':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;if ((connector->connector_type != DRM_MODE_CONNECTOR_DVII) &&(connector->connector_type != DRM_MODE_CONNECTOR_HDMIB))mode->force = DRM_FORCE_ON;elsemode->force = DRM_FORCE_ON_DIGITAL;break;case 'd':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;mode->force = DRM_FORCE_OFF;break;case 'e':if (mode->force != DRM_FORCE_UNSPECIFIED)return -EINVAL;mode->force = DRM_FORCE_ON;break;default:return -EINVAL;}}return 0;+}
+static int drm_mode_parse_cmdline_res_mode(const char *str, unsigned int length,
bool extras,struct drm_connector *connector,struct drm_cmdline_mode *mode)+{
bool rb = false, cvt = false;int xres = 0, yres = 0;int remaining, i;char *end_ptr;xres = simple_strtol(str, &end_ptr, 10);checkpatch is telling me to use kstrtol instead, as simple_strtol is deprecated
if (end_ptr[0] != 'x')check that end_ptr != NULL? you should probably also check that xres isn't an error (ie: -ERANGE or -EINVAL)
return -EINVAL;end_ptr++;yres = simple_strtol(end_ptr, &end_ptr, 10);check end_ptr != NULL and yres sane
remaining = length - (end_ptr - str);if (remaining < 0)right, so if end_ptr is NULL here, we'll end up with a huge positive value for remaining :)
return -EINVAL;for (i = 0; i < remaining; i++) {switch (end_ptr[i]) {case 'M':cvt = true;the previous code ensured proper ordering as well as parsing, whereas yours will take these in any order (and accepts duplicates). i don't think this should cause any issues, but perhaps something to verify.
Yes, I definitely overlooked the order of the parameters (and now I get why the switch was so convoluted...)
I'll come up with a second version fixing that (and the other comments you had).
Maxime
The drm subsystem also uses the video= kernel parameter, and in the documentation refers to the fbdev documentation for that parameter.
However, that documentation also says that instead of giving the mode using its resolution we can also give a name. However, DRM doesn't handle that case at the moment. Even though in most case it shouldn't make any difference, it might be useful for analog modes, where different standards might have the same resolution, but still have a few different parameters that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/drm_connector.c | 3 +- drivers/gpu/drm/drm_fb_helper.c | 4 +++- drivers/gpu/drm/drm_modes.c | 49 +++++++++++++++++++++++----------- include/drm/drm_connector.h | 1 +- 4 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 2db7fb510b6c..27a8a511257c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -147,8 +147,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) connector->force = mode->force; }
- DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n", + DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", connector->name, + mode->name ? mode->name : "", mode->xres, mode->yres, mode->refresh_specified ? mode->refresh : 60, mode->rb ? " reduced blanking" : "", diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bde1f15..20a68305fb45 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1748,6 +1748,10 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f prefer_non_interlace = !cmdline_mode->interlace; again: list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) { + /* Check (optional) mode name first */ + if (!strcmp(mode->name, cmdline_mode->name)) + return mode; + /* check width/height */ if (mode->hdisplay != cmdline_mode->xres || mode->vdisplay != cmdline_mode->yres) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 7d5bdca276f2..fdbf541a5978 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1413,7 +1413,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, struct drm_cmdline_mode *mode) { const char *name; - bool parse_extras = false; + bool named_mode = false, parse_extras = false; unsigned int bpp_off = 0, refresh_off = 0; unsigned int mode_end = 0; char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; @@ -1432,8 +1432,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
name = mode_option;
+ /* + * If the first character is not a digit, then it means that + * we have a named mode. + */ if (!isdigit(name[0])) - return false; + named_mode = true; + else + named_mode = false;
/* Try to locate the bpp and refresh specifiers, if any */ bpp_ptr = strchr(name, '-'); @@ -1460,12 +1466,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, parse_extras = true; }
- ret = drm_mode_parse_cmdline_res_mode(name, mode_end, - parse_extras, - connector, - mode); - if (ret) - return false; + if (named_mode) { + strncpy(mode->name, name, mode_end); + } else { + ret = drm_mode_parse_cmdline_res_mode(name, mode_end, + parse_extras, + connector, + mode); + if (ret) + return false; + } mode->specified = true;
if (bpp_ptr) { @@ -1493,14 +1503,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, extra_ptr = refresh_end_ptr;
if (extra_ptr) { - int remaining = strlen(name) - (extra_ptr - name); + if (!named_mode) { + int len = strlen(name) - (extra_ptr - name);
- /* - * We still have characters to process, while - * we shouldn't have any - */ - if (remaining > 0) - return false; + ret = drm_mode_parse_cmdline_extra(extra_ptr, len, + connector, mode); + if (ret) + return false; + } else { + int remaining = strlen(name) - (extra_ptr - name); + + /* + * We still have characters to process, while + * we shouldn't have any + */ + if (remaining > 0) + return false; + } }
return true; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ac9d7d8e0e43..dce3d4b2fd33 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -485,6 +485,7 @@ struct drm_connector_funcs {
/* mode specified on the command line */ struct drm_cmdline_mode { + char name[DRM_DISPLAY_MODE_LEN]; bool specified; bool refresh_specified; bool bpp_specified;
On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The drm subsystem also uses the video= kernel parameter, and in the documentation refers to the fbdev documentation for that parameter.
However, that documentation also says that instead of giving the mode using its resolution we can also give a name. However, DRM doesn't handle that case at the moment. Even though in most case it shouldn't make any difference, it might be useful for analog modes, where different standards might have the same resolution, but still have a few different parameters that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/drm_connector.c | 3 +- drivers/gpu/drm/drm_fb_helper.c | 4 +++- drivers/gpu/drm/drm_modes.c | 49 +++++++++++++++++++++++----------- include/drm/drm_connector.h | 1 +- 4 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 2db7fb510b6c..27a8a511257c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -147,8 +147,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) connector->force = mode->force; }
DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", connector->name,mode->name ? mode->name : "", mode->xres, mode->yres, mode->refresh_specified ? mode->refresh : 60, mode->rb ? " reduced blanking" : "",diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bde1f15..20a68305fb45 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1748,6 +1748,10 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f prefer_non_interlace = !cmdline_mode->interlace; again: list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
/* Check (optional) mode name first */if (!strcmp(mode->name, cmdline_mode->name))return mode;/* check width/height */ if (mode->hdisplay != cmdline_mode->xres || mode->vdisplay != cmdline_mode->yres)diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 7d5bdca276f2..fdbf541a5978 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1413,7 +1413,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, struct drm_cmdline_mode *mode) { const char *name;
bool parse_extras = false;
bool named_mode = false, parse_extras = false; unsigned int bpp_off = 0, refresh_off = 0; unsigned int mode_end = 0; char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;@@ -1432,8 +1432,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
name = mode_option;
/** If the first character is not a digit, then it means that* we have a named mode.*/ if (!isdigit(name[0]))
return false;
named_mode = true;elsenamed_mode = false;
named_mode = isalpha(name[0]); might be more succinct (and covers special characters).
/* Try to locate the bpp and refresh specifiers, if any */ bpp_ptr = strchr(name, '-');@@ -1460,12 +1466,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, parse_extras = true; }
ret = drm_mode_parse_cmdline_res_mode(name, mode_end,parse_extras,connector,mode);if (ret)return false;
if (named_mode) {strncpy(mode->name, name, mode_end);} else {ret = drm_mode_parse_cmdline_res_mode(name, mode_end,parse_extras,connector,mode);if (ret)return false;} mode->specified = true; if (bpp_ptr) {@@ -1493,14 +1503,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, extra_ptr = refresh_end_ptr;
if (extra_ptr) {
int remaining = strlen(name) - (extra_ptr - name);
if (!named_mode) {int len = strlen(name) - (extra_ptr - name);
/** We still have characters to process, while* we shouldn't have any*/if (remaining > 0)return false;
ret = drm_mode_parse_cmdline_extra(extra_ptr, len,connector, mode);if (ret)return false;} else {int remaining = strlen(name) - (extra_ptr - name);/** We still have characters to process, while* we shouldn't have any*/if (remaining > 0)return false;
Correct me if I'm wrong, but this shouldn't ever happen. AFAICT, the only way it could would be if we parsed bpp or refresh in the named mode (since those are the only cases where we don't copy the whole string over). Shouldn't that be invalid anyways?
Perhaps you can just avoid all of this code and just do a strcpy/return when you first detect a named mode.
Sean
} } return true;diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ac9d7d8e0e43..dce3d4b2fd33 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -485,6 +485,7 @@ struct drm_connector_funcs {
/* mode specified on the command line */ struct drm_cmdline_mode {
char name[DRM_DISPLAY_MODE_LEN]; bool specified; bool refresh_specified; bool bpp_specified;-- git-series 0.8.10
Hi Sean,
On Wed, Nov 16, 2016 at 12:21:42PM -0500, Sean Paul wrote:
On Tue, Oct 18, 2016 at 4:29 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
The drm subsystem also uses the video= kernel parameter, and in the documentation refers to the fbdev documentation for that parameter.
However, that documentation also says that instead of giving the mode using its resolution we can also give a name. However, DRM doesn't handle that case at the moment. Even though in most case it shouldn't make any difference, it might be useful for analog modes, where different standards might have the same resolution, but still have a few different parameters that are not encoded in the modes (NTSC vs NTSC-J vs PAL-M for example).
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
drivers/gpu/drm/drm_connector.c | 3 +- drivers/gpu/drm/drm_fb_helper.c | 4 +++- drivers/gpu/drm/drm_modes.c | 49 +++++++++++++++++++++++----------- include/drm/drm_connector.h | 1 +- 4 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 2db7fb510b6c..27a8a511257c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -147,8 +147,9 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) connector->force = mode->force; }
DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", connector->name,mode->name ? mode->name : "", mode->xres, mode->yres, mode->refresh_specified ? mode->refresh : 60, mode->rb ? " reduced blanking" : "",diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bde1f15..20a68305fb45 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1748,6 +1748,10 @@ struct drm_display_mode *drm_pick_cmdline_mode(struct drm_fb_helper_connector *f prefer_non_interlace = !cmdline_mode->interlace; again: list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) {
/* Check (optional) mode name first */if (!strcmp(mode->name, cmdline_mode->name))return mode;/* check width/height */ if (mode->hdisplay != cmdline_mode->xres || mode->vdisplay != cmdline_mode->yres)diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 7d5bdca276f2..fdbf541a5978 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1413,7 +1413,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, struct drm_cmdline_mode *mode) { const char *name;
bool parse_extras = false;
bool named_mode = false, parse_extras = false; unsigned int bpp_off = 0, refresh_off = 0; unsigned int mode_end = 0; char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;@@ -1432,8 +1432,14 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
name = mode_option;
/** If the first character is not a digit, then it means that* we have a named mode.*/ if (!isdigit(name[0]))
return false;
named_mode = true;elsenamed_mode = false;named_mode = isalpha(name[0]); might be more succinct (and covers special characters).
/* Try to locate the bpp and refresh specifiers, if any */ bpp_ptr = strchr(name, '-');@@ -1460,12 +1466,16 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, parse_extras = true; }
ret = drm_mode_parse_cmdline_res_mode(name, mode_end,parse_extras,connector,mode);if (ret)return false;
if (named_mode) {strncpy(mode->name, name, mode_end);} else {ret = drm_mode_parse_cmdline_res_mode(name, mode_end,parse_extras,connector,mode);if (ret)return false;} mode->specified = true; if (bpp_ptr) {@@ -1493,14 +1503,23 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, extra_ptr = refresh_end_ptr;
if (extra_ptr) {
int remaining = strlen(name) - (extra_ptr - name);
if (!named_mode) {int len = strlen(name) - (extra_ptr - name);
/** We still have characters to process, while* we shouldn't have any*/if (remaining > 0)return false;
ret = drm_mode_parse_cmdline_extra(extra_ptr, len,connector, mode);if (ret)return false;} else {int remaining = strlen(name) - (extra_ptr - name);/** We still have characters to process, while* we shouldn't have any*/if (remaining > 0)return false;Correct me if I'm wrong, but this shouldn't ever happen. AFAICT, the only way it could would be if we parsed bpp or refresh in the named mode (since those are the only cases where we don't copy the whole string over). Shouldn't that be invalid anyways?
It's supposed to be supported by the video string. Documentation/fb/modedb.txt defines:
Valid mode specifiers (mode_option argument):
<xres>x<yres>[M][R][-<bpp>][@<refresh>][i][m][eDd] <name>[-<bpp>][@<refresh>]
So we can't just copy the string over, and we need to parse it :/
Thanks! Maxime
We'll need a custom CRTC state to deal with the overscan setup.
We'll store in it the actual display size that can be used by the applications, and the size to use on the plane.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_backend.c | 18 +++++++++----- drivers/gpu/drm/sun4i/sun4i_crtc.c | 37 ++++++++++++++++++++++++++-- drivers/gpu/drm/sun4i/sun4i_crtc.h | 16 ++++++++++++- drivers/gpu/drm/sun4i/sun4i_rgb.c | 10 ++++++++- drivers/gpu/drm/sun4i/sun4i_tv.c | 14 +++++++++++- 5 files changed, 87 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 32c0584e3c35..9b36b7104c15 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -22,6 +22,7 @@ #include <linux/reset.h>
#include "sun4i_backend.h" +#include "sun4i_crtc.h" #include "sun4i_drv.h"
static u32 sunxi_rgb2yuv_coef[12] = { @@ -115,15 +116,19 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend, { struct drm_plane_state *state = plane->state; struct drm_framebuffer *fb = state->fb; + struct sun4i_crtc_state *s_state = drm_crtc_state_to_sun4i_crtc_state(state->crtc->state); + u16 x, y; +
DRM_DEBUG_DRIVER("Updating layer %d\n", layer);
if (plane->type == DRM_PLANE_TYPE_PRIMARY) { DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n", - state->crtc_w, state->crtc_h); + s_state->display_x_size, + s_state->display_y_size); regmap_write(backend->regs, SUN4I_BACKEND_DISSIZE_REG, - SUN4I_BACKEND_DISSIZE(state->crtc_w, - state->crtc_h)); + SUN4I_BACKEND_DISSIZE(s_state->display_x_size, + s_state->display_y_size)); }
/* Set the line width */ @@ -139,11 +144,12 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend, state->crtc_h));
/* Set base coordinates */ + x = s_state->plane_x_offset + state->crtc_x; + y = s_state->plane_y_offset + state->crtc_y; DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n", - state->crtc_x, state->crtc_y); + x, y); regmap_write(backend->regs, SUN4I_BACKEND_LAYCOOR_REG(layer), - SUN4I_BACKEND_LAYCOOR(state->crtc_x, - state->crtc_y)); + SUN4I_BACKEND_LAYCOOR(x, y));
return 0; } diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 4a192210574f..772e0ecd72db 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -104,9 +104,42 @@ static const struct drm_crtc_helper_funcs sun4i_crtc_helper_funcs = { .enable = sun4i_crtc_enable, };
+struct drm_crtc_state *sun4i_crtc_duplicate_state(struct drm_crtc *crtc) +{ + struct sun4i_crtc_state *state = drm_crtc_state_to_sun4i_crtc_state(crtc->state); + struct sun4i_crtc_state *copy; + + copy = kmalloc(sizeof(*copy), GFP_KERNEL); + if (!copy) + return NULL; + + DRM_DEBUG_DRIVER("Copying state %p to %p", state, copy); + + __drm_atomic_helper_crtc_duplicate_state(crtc, ©->base); + + copy->display_x_size = state->display_x_size; + copy->display_y_size = state->display_y_size; + + copy->plane_x_offset = state->plane_x_offset; + copy->plane_y_offset = state->plane_y_offset; + + return ©->base; +} + +void sun4i_crtc_destroy_state(struct drm_crtc *crtc, + struct drm_crtc_state *c_state) +{ + struct sun4i_crtc_state *s_state = drm_crtc_state_to_sun4i_crtc_state(c_state); + + DRM_DEBUG_DRIVER("Freeing state %p", s_state); + + __drm_atomic_helper_crtc_destroy_state(c_state); + kfree(s_state); +} + static const struct drm_crtc_funcs sun4i_crtc_funcs = { - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, + .atomic_destroy_state = sun4i_crtc_destroy_state, + .atomic_duplicate_state = sun4i_crtc_duplicate_state, .destroy = drm_crtc_cleanup, .page_flip = drm_atomic_helper_page_flip, .reset = drm_atomic_helper_crtc_reset, diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.h b/drivers/gpu/drm/sun4i/sun4i_crtc.h index dec8ce4d9b25..625c9ac41434 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.h +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.h @@ -20,11 +20,27 @@ struct sun4i_crtc { struct sun4i_drv *drv; };
+struct sun4i_crtc_state { + struct drm_crtc_state base; + + u32 display_x_size; + u32 display_y_size; + + u32 plane_x_offset; + u32 plane_y_offset; +}; + static inline struct sun4i_crtc *drm_crtc_to_sun4i_crtc(struct drm_crtc *crtc) { return container_of(crtc, struct sun4i_crtc, crtc); }
+static inline struct sun4i_crtc_state * +drm_crtc_state_to_sun4i_crtc_state(struct drm_crtc_state *state) +{ + return container_of(state, struct sun4i_crtc_state, base); +} + struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm);
#endif /* _SUN4I_CRTC_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index c3ff10f559cc..b1f792ad84c2 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -17,6 +17,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_panel.h>
+#include "sun4i_crtc.h" #include "sun4i_drv.h" #include "sun4i_tcon.h" #include "sun4i_rgb.h" @@ -141,6 +142,15 @@ static int sun4i_rgb_atomic_check(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) { + struct drm_display_mode *mode = &crtc_state->mode; + struct sun4i_crtc_state *state = drm_crtc_state_to_sun4i_crtc_state(crtc_state); + + state->display_x_size = mode->hdisplay; + state->display_y_size = mode->vdisplay; + + state->plane_x_offset = 0; + state->plane_y_offset = 0; + return 0; }
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c index 1dd3d9eabf2e..6f8077013be3 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tv.c +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c @@ -22,6 +22,7 @@ #include <drm/drm_panel.h>
#include "sun4i_backend.h" +#include "sun4i_crtc.h" #include "sun4i_drv.h" #include "sun4i_tcon.h"
@@ -343,6 +344,19 @@ static int sun4i_tv_atomic_check(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) { + struct drm_display_mode *mode = &crtc_state->mode; + const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode); + struct sun4i_crtc_state *state = drm_crtc_state_to_sun4i_crtc_state(crtc_state); + + if (!tv_mode) + return -EINVAL; + + state->display_x_size = tv_mode->hdisplay; + state->plane_x_offset = 0; + + state->display_y_size = tv_mode->vdisplay; + state->plane_y_offset = 0; + return 0; }
Since the mode passed in mode_set is probably going to be a scaled down version of the TV mode, we cannot just use it.
Instead, try to retrieve the actual mode we want to set, and generate a drm mode from that.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_tv.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c index 6f8077013be3..f99886462cb8 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tv.c +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c @@ -401,8 +401,13 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder, struct sun4i_drv *drv = tv->drv; struct sun4i_tcon *tcon = drv->tcon; const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode); + struct drm_display_mode tv_drm_mode = { 0 };
- sun4i_tcon1_mode_set(tcon, mode); + strcpy(tv_drm_mode.name, "TV"); + sun4i_tv_mode_to_drm_mode(tv_mode, &tv_drm_mode); + drm_mode_set_crtcinfo(&tv_drm_mode, CRTC_INTERLACE_HALVE_V); + + sun4i_tcon1_mode_set(tcon, &tv_drm_mode);
/* Enable and map the DAC to the output */ regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
Create overscan profiles reducing the displayed zone.
For each TV standard (PAL and NTSC so far), we create 4 more reduced modes by steps of 5% that the user will be able to select.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com --- drivers/gpu/drm/sun4i/sun4i_tv.c | 60 +++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c index f99886462cb8..9ee03ba086b6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tv.c +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c @@ -301,27 +301,33 @@ static const struct tv_mode *sun4i_tv_find_tv_by_mode(const struct drm_display_m DRM_DEBUG_DRIVER("Comparing mode %s vs %s", mode->name, tv_mode->name);
- if (!strcmp(mode->name, tv_mode->name)) + if (!strncmp(mode->name, tv_mode->name, strlen(tv_mode->name))) return tv_mode; }
/* Then by number of lines */ for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { const struct tv_mode *tv_mode = &tv_modes[i]; + int j;
- DRM_DEBUG_DRIVER("Comparing mode %s vs %s (X: %d vs %d)", - mode->name, tv_mode->name, - mode->vdisplay, tv_mode->vdisplay); + for (j = 0; j < 20; j += 5) { + u32 vdisplay = tv_mode->vdisplay * (100 - j) / 100;
- if (mode->vdisplay == tv_mode->vdisplay) - return tv_mode; + DRM_DEBUG_DRIVER("Comparing mode with %s (%d) (X: %d vs %d)", + tv_mode->name, j, + vdisplay, tv_mode->vdisplay); + + if (vdisplay == tv_mode->vdisplay) + return tv_mode; + } }
return NULL; }
static void sun4i_tv_mode_to_drm_mode(const struct tv_mode *tv_mode, - struct drm_display_mode *mode) + struct drm_display_mode *mode, + int overscan) { DRM_DEBUG_DRIVER("Creating mode %s\n", mode->name);
@@ -329,12 +335,12 @@ static void sun4i_tv_mode_to_drm_mode(const struct tv_mode *tv_mode, mode->clock = 13500; mode->flags = DRM_MODE_FLAG_INTERLACE;
- mode->hdisplay = tv_mode->hdisplay; + mode->hdisplay = tv_mode->hdisplay * (100 - overscan) / 100; mode->hsync_start = mode->hdisplay + tv_mode->hfront_porch; mode->hsync_end = mode->hsync_start + tv_mode->hsync_len; mode->htotal = mode->hsync_end + tv_mode->hback_porch;
- mode->vdisplay = tv_mode->vdisplay; + mode->vdisplay = tv_mode->vdisplay * (100 - overscan) / 100; mode->vsync_start = mode->vdisplay + tv_mode->vfront_porch; mode->vsync_end = mode->vsync_start + tv_mode->vsync_len; mode->vtotal = mode->vsync_end + tv_mode->vback_porch; @@ -352,10 +358,10 @@ static int sun4i_tv_atomic_check(struct drm_encoder *encoder, return -EINVAL;
state->display_x_size = tv_mode->hdisplay; - state->plane_x_offset = 0; + state->plane_x_offset = (tv_mode->hdisplay - mode->hdisplay) / 2;
state->display_y_size = tv_mode->vdisplay; - state->plane_y_offset = 0; + state->plane_y_offset = (tv_mode->vdisplay - mode->vdisplay) / 2;
return 0; } @@ -404,7 +410,7 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode tv_drm_mode = { 0 };
strcpy(tv_drm_mode.name, "TV"); - sun4i_tv_mode_to_drm_mode(tv_mode, &tv_drm_mode); + sun4i_tv_mode_to_drm_mode(tv_mode, &tv_drm_mode, 0); drm_mode_set_crtcinfo(&tv_drm_mode, CRTC_INTERLACE_HALVE_V);
sun4i_tcon1_mode_set(tcon, &tv_drm_mode); @@ -526,22 +532,28 @@ static int sun4i_tv_comp_get_modes(struct drm_connector *connector) int i;
for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { - struct drm_display_mode *mode; const struct tv_mode *tv_mode = &tv_modes[i]; - - mode = drm_mode_create(connector->dev); - if (!mode) { - DRM_ERROR("Failed to create a new display mode\n"); - return 0; + int j; + + for (j = 0; j < 20; j += 5) { + struct drm_display_mode *mode = drm_mode_create(connector->dev); + if (!mode) { + DRM_ERROR("Failed to create a new display mode\n"); + return 0; + } + + if (j) + sprintf(mode->name, "%s%d", tv_mode->name, + j); + else + strcpy(mode->name, tv_mode->name); + + sun4i_tv_mode_to_drm_mode(tv_mode, mode, j); + drm_mode_probed_add(connector, mode); } - - strcpy(mode->name, tv_mode->name); - - sun4i_tv_mode_to_drm_mode(tv_mode, mode); - drm_mode_probed_add(connector, mode); }
- return i; + return i * 4; }
static int sun4i_tv_comp_mode_valid(struct drm_connector *connector,
On Tue, Oct 18, 2016 at 10:29:38AM +0200, Maxime Ripard wrote:
Create overscan profiles reducing the displayed zone.
For each TV standard (PAL and NTSC so far), we create 4 more reduced modes by steps of 5% that the user will be able to select.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
tbh I think if we agree to do this (and that still seems an open question) I think there should be a generic helper to add these overscan modes with increased porches. Anything that only depends upon the sink (and overscanning is something the sink does) should imo be put into a suitable helper library for everyone to share.
Or maybe even stash it into the probe helpers and call it for all TV connectors. Definitely not a driver-private thing. -Daniel
drivers/gpu/drm/sun4i/sun4i_tv.c | 60 +++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c index f99886462cb8..9ee03ba086b6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tv.c +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c @@ -301,27 +301,33 @@ static const struct tv_mode *sun4i_tv_find_tv_by_mode(const struct drm_display_m DRM_DEBUG_DRIVER("Comparing mode %s vs %s", mode->name, tv_mode->name);
if (!strcmp(mode->name, tv_mode->name))
if (!strncmp(mode->name, tv_mode->name, strlen(tv_mode->name))) return tv_mode;}
/* Then by number of lines */ for (i = 0; i < ARRAY_SIZE(tv_modes); i++) { const struct tv_mode *tv_mode = &tv_modes[i];
int j;
DRM_DEBUG_DRIVER("Comparing mode %s vs %s (X: %d vs %d)",mode->name, tv_mode->name,mode->vdisplay, tv_mode->vdisplay);
for (j = 0; j < 20; j += 5) {u32 vdisplay = tv_mode->vdisplay * (100 - j) / 100;
if (mode->vdisplay == tv_mode->vdisplay)return tv_mode;
DRM_DEBUG_DRIVER("Comparing mode with %s (%d) (X: %d vs %d)",tv_mode->name, j,vdisplay, tv_mode->vdisplay);if (vdisplay == tv_mode->vdisplay)return tv_mode;}}
return NULL;
}
static void sun4i_tv_mode_to_drm_mode(const struct tv_mode *tv_mode,
struct drm_display_mode *mode)
struct drm_display_mode *mode,int overscan){ DRM_DEBUG_DRIVER("Creating mode %s\n", mode->name);
@@ -329,12 +335,12 @@ static void sun4i_tv_mode_to_drm_mode(const struct tv_mode *tv_mode, mode->clock = 13500; mode->flags = DRM_MODE_FLAG_INTERLACE;
- mode->hdisplay = tv_mode->hdisplay;
- mode->hdisplay = tv_mode->hdisplay * (100 - overscan) / 100; mode->hsync_start = mode->hdisplay + tv_mode->hfront_porch; mode->hsync_end = mode->hsync_start + tv_mode->hsync_len; mode->htotal = mode->hsync_end + tv_mode->hback_porch;
- mode->vdisplay = tv_mode->vdisplay;
- mode->vdisplay = tv_mode->vdisplay * (100 - overscan) / 100; mode->vsync_start = mode->vdisplay + tv_mode->vfront_porch; mode->vsync_end = mode->vsync_start + tv_mode->vsync_len; mode->vtotal = mode->vsync_end + tv_mode->vback_porch;
@@ -352,10 +358,10 @@ static int sun4i_tv_atomic_check(struct drm_encoder *encoder, return -EINVAL;
state->display_x_size = tv_mode->hdisplay;
- state->plane_x_offset = 0;
state->plane_x_offset = (tv_mode->hdisplay - mode->hdisplay) / 2;
state->display_y_size = tv_mode->vdisplay;
- state->plane_y_offset = 0;
state->plane_y_offset = (tv_mode->vdisplay - mode->vdisplay) / 2;
return 0;
} @@ -404,7 +410,7 @@ static void sun4i_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode tv_drm_mode = { 0 };
strcpy(tv_drm_mode.name, "TV");
- sun4i_tv_mode_to_drm_mode(tv_mode, &tv_drm_mode);
sun4i_tv_mode_to_drm_mode(tv_mode, &tv_drm_mode, 0); drm_mode_set_crtcinfo(&tv_drm_mode, CRTC_INTERLACE_HALVE_V);
sun4i_tcon1_mode_set(tcon, &tv_drm_mode);
@@ -526,22 +532,28 @@ static int sun4i_tv_comp_get_modes(struct drm_connector *connector) int i;
for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
struct drm_display_mode *mode;mode = drm_mode_create(connector->dev);if (!mode) {DRM_ERROR("Failed to create a new display mode\n");return 0;
int j;for (j = 0; j < 20; j += 5) {struct drm_display_mode *mode = drm_mode_create(connector->dev);if (!mode) {DRM_ERROR("Failed to create a new display mode\n");return 0;}if (j)sprintf(mode->name, "%s%d", tv_mode->name,j);elsestrcpy(mode->name, tv_mode->name);sun4i_tv_mode_to_drm_mode(tv_mode, mode, j);drm_mode_probed_add(connector, mode);
strcpy(mode->name, tv_mode->name);sun4i_tv_mode_to_drm_mode(tv_mode, mode);drm_mode_probed_add(connector, mode);}
return i;
- return i * 4;
}
static int sun4i_tv_comp_mode_valid(struct drm_connector *connector,
git-series 0.8.10 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel,
On Tue, Nov 08, 2016 at 09:59:27AM +0100, Daniel Vetter wrote:
On Tue, Oct 18, 2016 at 10:29:38AM +0200, Maxime Ripard wrote:
Create overscan profiles reducing the displayed zone.
For each TV standard (PAL and NTSC so far), we create 4 more reduced modes by steps of 5% that the user will be able to select.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
tbh I think if we agree to do this (and that still seems an open question) I think there should be a generic helper to add these overscan modes with increased porches. Anything that only depends upon the sink (and overscanning is something the sink does) should imo be put into a suitable helper library for everyone to share.
Or maybe even stash it into the probe helpers and call it for all TV connectors. Definitely not a driver-private thing.
Last time we discussed it, my recollection was that you didn't want to have generic code for it, but I'd be happy to implement it.
I'll come up with something like that.
Thanks! Maxime
On Thu, Nov 10, 2016 at 03:56:30PM +0100, Maxime Ripard wrote:
Hi Daniel,
On Tue, Nov 08, 2016 at 09:59:27AM +0100, Daniel Vetter wrote:
On Tue, Oct 18, 2016 at 10:29:38AM +0200, Maxime Ripard wrote:
Create overscan profiles reducing the displayed zone.
For each TV standard (PAL and NTSC so far), we create 4 more reduced modes by steps of 5% that the user will be able to select.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
tbh I think if we agree to do this (and that still seems an open question) I think there should be a generic helper to add these overscan modes with increased porches. Anything that only depends upon the sink (and overscanning is something the sink does) should imo be put into a suitable helper library for everyone to share.
Or maybe even stash it into the probe helpers and call it for all TV connectors. Definitely not a driver-private thing.
Last time we discussed it, my recollection was that you didn't want to have generic code for it, but I'd be happy to implement it.
I'll come up with something like that.
Well I can flip-flop around with the nonsense I'm sometimes emitting ;-) Since you called me out, feel free to do whatever you want ... -Daniel
On Fri, Nov 11, 2016 at 10:17:55AM +0100, Daniel Vetter wrote:
On Thu, Nov 10, 2016 at 03:56:30PM +0100, Maxime Ripard wrote:
Hi Daniel,
On Tue, Nov 08, 2016 at 09:59:27AM +0100, Daniel Vetter wrote:
On Tue, Oct 18, 2016 at 10:29:38AM +0200, Maxime Ripard wrote:
Create overscan profiles reducing the displayed zone.
For each TV standard (PAL and NTSC so far), we create 4 more reduced modes by steps of 5% that the user will be able to select.
Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com
tbh I think if we agree to do this (and that still seems an open question) I think there should be a generic helper to add these overscan modes with increased porches. Anything that only depends upon the sink (and overscanning is something the sink does) should imo be put into a suitable helper library for everyone to share.
Or maybe even stash it into the probe helpers and call it for all TV connectors. Definitely not a driver-private thing.
Last time we discussed it, my recollection was that you didn't want to have generic code for it, but I'd be happy to implement it.
I'll come up with something like that.
Well I can flip-flop around with the nonsense I'm sometimes emitting ;-) Since you called me out, feel free to do whatever you want ...
I also found the generic solution to be a much better solution, so I'll definitely implement it :)
Maxime
On Tue, Oct 18, 2016 at 10:29:33AM +0200, Maxime Ripard wrote:
The Allwinner display engine doesn't have any kind of hardware help to deal with TV overscan.
I'm not sure I follow. My understanding (from reading the CEA specs) is that TVs are expected to overscan the image, so the upper left, and bottom right pixels are not visible.
I assume we are talking about TVs connected via HDMI. In the HDMI AVI infoframe, there are bits which specify whether the image should be overscanned or underscanned - however, whether a TV implements those bits is rather sketchy. I assume when you say "any kind of hardware help" you mean you can't control these bits?
However, some (most?) TVs now implement a menu option which allows the (over)scan mode to be selected. Others assume that if it's a TV mode, it's supposed to be overscanned, if it's a "PC" mode, it should be underscanned and provide no option to change the behaviour.
This means that if we use the only mode the hardware provides (either PAL or NTSC, or something else), most of the screens will crop the borders of the image, which is bad.
I think you're trying to apply monitor-type behaviour to TVs...
We can however use somekind of a hack, to instead reduce the mode exposed to the userspace, and center it in the final image. We would expose different overscan ratio to be able to deal with most of the screens, each reducing more the displayable area.
I'm not sure we need "a hack". What if we treated the primary plane just like any other (eg, overlay) plane? We could then specify (eg) a 1920x1080 display mode, but with the primary plane reduced in size, positioned in the centre of the display mode?
I know that there's hardware out there which can do exactly that - Marvell Dove implements this: you set the display size separately from two planes, one graphics plane and one video plane. Both planes can be positioned anywhere in the displayed size.
We could then specify at DRM level that a connected device overscans by N%, and have the primary plane adjusted by DRM itself.
Hi Russell,
On Tue, Oct 18, 2016 at 10:24:22AM +0100, Russell King - ARM Linux wrote:
On Tue, Oct 18, 2016 at 10:29:33AM +0200, Maxime Ripard wrote:
The Allwinner display engine doesn't have any kind of hardware help to deal with TV overscan.
I'm not sure I follow. My understanding (from reading the CEA specs) is that TVs are expected to overscan the image, so the upper left, and bottom right pixels are not visible.
Yes, this is why we have to work around it somehow.
I assume we are talking about TVs connected via HDMI. In the HDMI AVI infoframe, there are bits which specify whether the image should be overscanned or underscanned - however, whether a TV implements those bits is rather sketchy. I assume when you say "any kind of hardware help" you mean you can't control these bits?
However, some (most?) TVs now implement a menu option which allows the (over)scan mode to be selected. Others assume that if it's a TV mode, it's supposed to be overscanned, if it's a "PC" mode, it should be underscanned and provide no option to change the behaviour.
We're talking about plain dumb composite output, so no infoframes, setup or anything here :)
This means that if we use the only mode the hardware provides (either PAL or NTSC, or something else), most of the screens will crop the borders of the image, which is bad.
I think you're trying to apply monitor-type behaviour to TVs...
Yes, kind of. Our users are usually running a desktop distro, and the default output on those boards are just plain composite, which means running any DE onto a TV.
Note that it's not only about the interface itself, but you'll lose content for all pictures displayed. And no one cares about the TV safe area anymore these days (starting with the framebuffer console).
We can however use somekind of a hack, to instead reduce the mode exposed to the userspace, and center it in the final image. We would expose different overscan ratio to be able to deal with most of the screens, each reducing more the displayable area.
I'm not sure we need "a hack". What if we treated the primary plane just like any other (eg, overlay) plane? We could then specify (eg) a 1920x1080 display mode, but with the primary plane reduced in size, positioned in the centre of the display mode?
I know that there's hardware out there which can do exactly that - Marvell Dove implements this: you set the display size separately from two planes, one graphics plane and one video plane. Both planes can be positioned anywhere in the displayed size.
This might have been poorly worded on my side, but it's exactly what I'm doing in those patches.
We could then specify at DRM level that a connected device overscans by N%, and have the primary plane adjusted by DRM itself.
I'd agree with you, however, there's a few issues with that I think.
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
The fourth one being the major one. Every time I raised the issue on IRC, the answer basically was "we don't care about analog", so I'm a bit pessimistic about whether dealing with this in the core would be accepted, hence why I chose to deal with this at the driver level.
Thanks, Maxime
On Tue, 18 Oct 2016 12:03:49 +0200 Maxime Ripard maxime.ripard@free-electrons.com wrote:
The fourth one being the major one. Every time I raised the issue on IRC, the answer basically was "we don't care about analog", so I'm a bit pessimistic about whether dealing with this in the core would be accepted, hence why I chose to deal with this at the driver level.
The same problem exists with HDMI and old TVs (mine is an ASUS 22T1E): these TVs overscan as soon as AVI frames are in the stream.
On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
See xbmc... they go through a nice shiny setup which includes adjusting the visible area. From what I remember, it has pointers on each corner which you can adjust to be just visible on the screen, so xbmc knows how much overscan there is, and xbmc itself reduces down to the user set size.
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
I'm not sure about that - we want the graphics to be visible, but that may not be appropriate for an video overlay frame. It's quite common for (eg) broadcast video to contain dead pixels or other artifacts on the right hand side, and the broadcast video expects overscan to be present.
I know this because I have run my TV with overscan disabled, even for broadcast TV.
The fourth one being the major one. Every time I raised the issue on IRC, the answer basically was "we don't care about analog", so I'm a bit pessimistic about whether dealing with this in the core would be accepted, hence why I chose to deal with this at the driver level.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
Hi Russell,
On Mon, Oct 31, 2016 at 08:42:34AM +0000, Russell King - ARM Linux wrote:
On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
See xbmc... they go through a nice shiny setup which includes adjusting the visible area. From what I remember, it has pointers on each corner which you can adjust to be just visible on the screen, so xbmc knows how much overscan there is, and xbmc itself reduces down to the user set size.
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
We might not have a GPU in the system, and we might not even have an entire graphic stack on top either, so I don't think fixing at the user-space level is a good option (especially since we already have an overscan property in DRM).
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
I'm not sure about that - we want the graphics to be visible, but that may not be appropriate for an video overlay frame. It's quite common for (eg) broadcast video to contain dead pixels or other artifacts on the right hand side, and the broadcast video expects overscan to be present.
I know this because I have run my TV with overscan disabled, even for broadcast TV.
I know, but on this particular hardware, composite really is just another video output. There's not even a TV receiver in it, so I don't think we have to worry about it.
The fourth one being the major one. Every time I raised the issue on IRC, the answer basically was "we don't care about analog", so I'm a bit pessimistic about whether dealing with this in the core would be accepted, hence why I chose to deal with this at the driver level.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
I'd agree with you, but I was also told to not turn that into a generic code and deal with that in my driver.
Do you have any suggestions?
Thanks, Maxime
On Thu, Nov 03, 2016 at 10:01:06AM +0100, Maxime Ripard wrote:
Hi Russell,
On Mon, Oct 31, 2016 at 08:42:34AM +0000, Russell King - ARM Linux wrote:
On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
See xbmc... they go through a nice shiny setup which includes adjusting the visible area. From what I remember, it has pointers on each corner which you can adjust to be just visible on the screen, so xbmc knows how much overscan there is, and xbmc itself reduces down to the user set size.
I was replying to your comment "so we need one way to let the user tell how the image is displayed".
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
Well, it will have to be doing a scaling step anyway. If the video frame is a different size to the active area, scaling is required no matter what. A 576p SD image needs to be scaled up, and a 1080p image would need to be scaled down for a 1080p overscanned display with a reduced active area to counter the overscanning - no matter how you do it.
For graphics, userspace could add mode(s) with increased porches and reduced active area itself to achieve an underscanned display on a timing which the display device always overscans - there's no need to do that in the kernel, all the APIs are there to be able to do it already.
That means your framebuffer will be smaller, but that's the case anyway.
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
I'm not sure about that - we want the graphics to be visible, but that may not be appropriate for an video overlay frame. It's quite common for (eg) broadcast video to contain dead pixels or other artifacts on the right hand side, and the broadcast video expects overscan to be present.
I know this because I have run my TV with overscan disabled, even for broadcast TV.
I know, but on this particular hardware, composite really is just another video output. There's not even a TV receiver in it, so I don't think we have to worry about it.
Whether or not there's a TV receiver is irrelevant - if you can decode broadcast video, then you run into the problem. For example, if you plug in a USB DVB stick, stream it across the network, etc.
Remember, you're proposing a generic solution, so making arguments about things that are not possible with your specific hardware isn't really that relevant to a generic solution.
So, I may want my graphics to appear on an overscanned display such that I can see the borders, but I may want the overlaid video to use the full active area (including overscan) to hide some of the broadcast mess at the edges of the screen.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
I'd agree with you, but I was also told to not turn that into a generic code and deal with that in my driver.
I guess whoever told you that was wrong. I used to have a cheap TV here which took HDMI, and always overscans broadcast modes, and underscans PC modes. No amount of fiddling with various settings (either in the TV or AVI frames) changes that.
I've run into that with a _monitor_ that Andrew Hutton has, which has a HDMI input - exactly the same thing - 1080p, 720p etc are all unconditionally overscanned, 1024x768 etc are all unconditionally underscanned and there's nothing that can be done to change it.
The problem is that TVs are at liberty to have this behaviour, so the overscaned problem is always going to be there, and each driver should not be dealing with it in their own specific ways.
Hi Russell,
On Thu, Nov 03, 2016 at 09:54:45AM +0000, Russell King - ARM Linux wrote:
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
Well, it will have to be doing a scaling step anyway. If the video frame is a different size to the active area, scaling is required no matter what. A 576p SD image needs to be scaled up, and a 1080p image would need to be scaled down for a 1080p overscanned display with a reduced active area to counter the overscanning - no matter how you do it.
Yes, except that scaling is not always an option. In my particular example, there's no scaler after the CRTC, which essentially prevents it from being used in that use case. Which is also why I ended up reducing the mode reported to the user.
For graphics, userspace could add mode(s) with increased porches and reduced active area itself to achieve an underscanned display on a timing which the display device always overscans - there's no need to do that in the kernel, all the APIs are there to be able to do it already.
That means your framebuffer will be smaller, but that's the case anyway.
Yes, that would be a good idea. But it's not always an option for applications that would rely on the fbdev emulation (like QT's eglfs), which would then have no way to change whatever default there is (and the only one able to know how bad it actually is is the user).
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
I'm not sure about that - we want the graphics to be visible, but that may not be appropriate for an video overlay frame. It's quite common for (eg) broadcast video to contain dead pixels or other artifacts on the right hand side, and the broadcast video expects overscan to be present.
I know this because I have run my TV with overscan disabled, even for broadcast TV.
I know, but on this particular hardware, composite really is just another video output. There's not even a TV receiver in it, so I don't think we have to worry about it.
Whether or not there's a TV receiver is irrelevant - if you can decode broadcast video, then you run into the problem. For example, if you plug in a USB DVB stick, stream it across the network, etc.
Good point.
Remember, you're proposing a generic solution, so making arguments about things that are not possible with your specific hardware isn't really that relevant to a generic solution.
This is definitely not what I'm proposing. I've been told very early on that such a generic solution was not even worth working on.
So, I may want my graphics to appear on an overscanned display such that I can see the borders, but I may want the overlaid video to use the full active area (including overscan) to hide some of the broadcast mess at the edges of the screen.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
I'd agree with you, but I was also told to not turn that into a generic code and deal with that in my driver.
I guess whoever told you that was wrong. I used to have a cheap TV here which took HDMI, and always overscans broadcast modes, and underscans PC modes. No amount of fiddling with various settings (either in the TV or AVI frames) changes that.
I've run into that with a _monitor_ that Andrew Hutton has, which has a HDMI input - exactly the same thing - 1080p, 720p etc are all unconditionally overscanned, 1024x768 etc are all unconditionally underscanned and there's nothing that can be done to change it.
The problem is that TVs are at liberty to have this behaviour, so the overscaned problem is always going to be there, and each driver should not be dealing with it in their own specific ways.
I agree with you, however, without any directions on how to do this, and willingness to merge this, I don't really see why we would work on such a generic implementation in the first place.
Maxime
On Mon, Nov 07, 2016 at 04:09:09PM +0100, Maxime Ripard wrote:
Hi Russell,
On Thu, Nov 03, 2016 at 09:54:45AM +0000, Russell King - ARM Linux wrote:
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
Well, it will have to be doing a scaling step anyway. If the video frame is a different size to the active area, scaling is required no matter what. A 576p SD image needs to be scaled up, and a 1080p image would need to be scaled down for a 1080p overscanned display with a reduced active area to counter the overscanning - no matter how you do it.
Yes, except that scaling is not always an option. In my particular example, there's no scaler after the CRTC, which essentially prevents it from being used in that use case. Which is also why I ended up reducing the mode reported to the user.
I think you completely missed my point. Let me try again.
If you expose a reduced mode to the user, you are reporting that (eg) the 1080p-timings mode does not have 1920 pixels horizontally, and 1080 lines. You are instead reporting that it has (eg) 1800 pixels horizontally and maybe 1000 lines.
So, when you play back a 1080 video, you are going to have to either:
1. crop the extra 120 pixels horizontally and 80 lines vertically or 2. scale the image.
However, this is a completely independent issue to how we go about setting a video mode that is 1800x1000 in the first place.
What you're suggesting is that we add code to the kernel to report that your non-EDID, analogue output transforms the standard 1920x1080 timings such that it has a 1800x1000 active area.
I'm suggesting instead that you can do the same thing in userspace by specifically adding a mode which has the 1920x1080 standard timings, but with the porches increased and the active area reduced - in exactly the same way that you'd have to do within the kernel to report your active-area-reduced 1800x1000 mode.
For graphics, userspace could add mode(s) with increased porches and reduced active area itself to achieve an underscanned display on a timing which the display device always overscans - there's no need to do that in the kernel, all the APIs are there to be able to do it already.
That means your framebuffer will be smaller, but that's the case anyway.
Yes, that would be a good idea. But it's not always an option for applications that would rely on the fbdev emulation (like QT's eglfs), which would then have no way to change whatever default there is (and the only one able to know how bad it actually is is the user).
I guess this is the problem with DRM people wanting to deprecate fbdev... too much stuff currently relies upon it, but DRM on x86 has always had the reduced functionality.
I guess there's two solutions here:
1. Either DRMs fbdev gains increased functionality, or 2. The fbdev-only applications/libraries need to be ported over to support DRM natively.
This has been a bar for some time to moving over to DRM, and I've heard very little desire on either side to find some sort of compromise on the issue, so I guess things are rather stuck where they are.
So, I may want my graphics to appear on an overscanned display such that I can see the borders, but I may want the overlaid video to use the full active area (including overscan) to hide some of the broadcast mess at the edges of the screen.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
I'd agree with you, but I was also told to not turn that into a generic code and deal with that in my driver.
I guess whoever told you that was wrong. I used to have a cheap TV here which took HDMI, and always overscans broadcast modes, and underscans PC modes. No amount of fiddling with various settings (either in the TV or AVI frames) changes that.
I've run into that with a _monitor_ that Andrew Hutton has, which has a HDMI input - exactly the same thing - 1080p, 720p etc are all unconditionally overscanned, 1024x768 etc are all unconditionally underscanned and there's nothing that can be done to change it.
The problem is that TVs are at liberty to have this behaviour, so the overscaned problem is always going to be there, and each driver should not be dealing with it in their own specific ways.
I agree with you, however, without any directions on how to do this, and willingness to merge this, I don't really see why we would work on such a generic implementation in the first place.
As I've already said - if you've been told that a generic implementation is wrong, then I suspect that either those who told you that either failed to understand the problem, or they failed to appreciate just what a problem it is.
I guess if DRM folk want to keep throwing up blocks in the way to moving forward, then we'll end up with lots of hacks in various random trees to work around the various issues, and everything will end up in a total mess.
This is _not_ a driver specific problem, it _is_ a _generic_ problem.
I guess DRM folk have nice expensive display devices which don't exhibit the problem, so they just don't give a damn - which basically means they're out of touch with the rest of the world. This isn't a problem that's going to go away just because DRM folk want it to, and it's not limited to "obsolete analogue" either.
I'd go as far as to say that DRM people are totally wrong to tell you that a solution to this should not be generic, period. I think I've made that point several times through the course of this discussion already.
On Mon, Nov 07, 2016 at 03:46:52PM +0000, Russell King - ARM Linux wrote:
On Mon, Nov 07, 2016 at 04:09:09PM +0100, Maxime Ripard wrote:
Hi Russell,
On Thu, Nov 03, 2016 at 09:54:45AM +0000, Russell King - ARM Linux wrote:
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
Well, it will have to be doing a scaling step anyway. If the video frame is a different size to the active area, scaling is required no matter what. A 576p SD image needs to be scaled up, and a 1080p image would need to be scaled down for a 1080p overscanned display with a reduced active area to counter the overscanning - no matter how you do it.
Yes, except that scaling is not always an option. In my particular example, there's no scaler after the CRTC, which essentially prevents it from being used in that use case. Which is also why I ended up reducing the mode reported to the user.
I think you completely missed my point. Let me try again.
If you expose a reduced mode to the user, you are reporting that (eg) the 1080p-timings mode does not have 1920 pixels horizontally, and 1080 lines. You are instead reporting that it has (eg) 1800 pixels horizontally and maybe 1000 lines.
So, when you play back a 1080 video, you are going to have to either:
- crop the extra 120 pixels horizontally and 80 lines vertically
or 2. scale the image.
However, this is a completely independent issue to how we go about setting a video mode that is 1800x1000 in the first place.
What you're suggesting is that we add code to the kernel to report that your non-EDID, analogue output transforms the standard 1920x1080 timings such that it has a 1800x1000 active area.
I'm suggesting instead that you can do the same thing in userspace by specifically adding a mode which has the 1920x1080 standard timings, but with the porches increased and the active area reduced - in exactly the same way that you'd have to do within the kernel to report your active-area-reduced 1800x1000 mode.
Ah, yes, you meant input scaling, not output, sorry.
For graphics, userspace could add mode(s) with increased porches and reduced active area itself to achieve an underscanned display on a timing which the display device always overscans - there's no need to do that in the kernel, all the APIs are there to be able to do it already.
That means your framebuffer will be smaller, but that's the case anyway.
Yes, that would be a good idea. But it's not always an option for applications that would rely on the fbdev emulation (like QT's eglfs), which would then have no way to change whatever default there is (and the only one able to know how bad it actually is is the user).
I guess this is the problem with DRM people wanting to deprecate fbdev... too much stuff currently relies upon it, but DRM on x86 has always had the reduced functionality.
I guess there's two solutions here:
- Either DRMs fbdev gains increased functionality, or
- The fbdev-only applications/libraries need to be ported over to support DRM natively.
This has been a bar for some time to moving over to DRM, and I've heard very little desire on either side to find some sort of compromise on the issue, so I guess things are rather stuck where they are.
I guess it really all boils down to this, and whether the userspace will be able to set a custom mode on its own. "Advanced" stacks like Xorg and Wayland will, but simpler and / or legacy applications will depend on the fbdev emulation, either because they've not been converted through DRM (like you suggested) or because it depends on a blob that requires it (and then you're stuck).
And since the kernel already deals with overscan through a generic property, it really feels like it's the place it should be done to address all needs (and ideally in a generic way).
Maxime
On Thu, Nov 3, 2016 at 3:01 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi Russell,
On Mon, Oct 31, 2016 at 08:42:34AM +0000, Russell King - ARM Linux wrote:
On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
See xbmc... they go through a nice shiny setup which includes adjusting the visible area. From what I remember, it has pointers on each corner which you can adjust to be just visible on the screen, so xbmc knows how much overscan there is, and xbmc itself reduces down to the user set size.
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
We might not have a GPU in the system, and we might not even have an entire graphic stack on top either, so I don't think fixing at the user-space level is a good option (especially since we already have an overscan property in DRM).
Hi Maxime, I took a quick look at the first 2 patches in the series and they look good at first glance. I have them in my queue to review more carefully.
Can you explain why you can't fix this by specifying a new mode with big porches (as Russell suggested)?
Sean
The second one is that we still need to expose the reduced modes to userspace, and not only the displayed size, so that the applications know what they must draw on. But I guess this could be adjusted by the core too.
In order to work consistently, I think all planes should be adjusted that way, so that relative coordinates are from the primary plane origin, and not the display origin. But that could be adjusted too by the core I guess.
I'm not sure about that - we want the graphics to be visible, but that may not be appropriate for an video overlay frame. It's quite common for (eg) broadcast video to contain dead pixels or other artifacts on the right hand side, and the broadcast video expects overscan to be present.
I know this because I have run my TV with overscan disabled, even for broadcast TV.
I know, but on this particular hardware, composite really is just another video output. There's not even a TV receiver in it, so I don't think we have to worry about it.
The fourth one being the major one. Every time I raised the issue on IRC, the answer basically was "we don't care about analog", so I'm a bit pessimistic about whether dealing with this in the core would be accepted, hence why I chose to deal with this at the driver level.
Yea, that's quite sad, "analog" has become a dirty word, but really this has nothing to do with "analog" at all - there are LCD TVs (and some monitors) out there which take HDMI signals but refuse to disable overscan no matter what you do to them if you provide them with a "broadcast" mode - so the analog excuse is very poor.
I'd agree with you, but I was also told to not turn that into a generic code and deal with that in my driver.
Do you have any suggestions?
Thanks, Maxime
-- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Hi Sean,
On Thu, Nov 03, 2016 at 03:11:26PM -0600, Sean Paul wrote:
On Thu, Nov 3, 2016 at 3:01 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
Hi Russell,
On Mon, Oct 31, 2016 at 08:42:34AM +0000, Russell King - ARM Linux wrote:
On Tue, Oct 18, 2016 at 12:03:49PM +0200, Maxime Ripard wrote:
The first one is that this overscanning should be reported by the connector I guess? but this is really TV specific, so we need one way to let the user tell how the image is displayed on its side, and we cannot really autodetect it, and this needs to be done at runtime so that we can present some shiny interface to let it select which overscan ratio works for him/her.
See xbmc... they go through a nice shiny setup which includes adjusting the visible area. From what I remember, it has pointers on each corner which you can adjust to be just visible on the screen, so xbmc knows how much overscan there is, and xbmc itself reduces down to the user set size.
Yes. And that is an XBMC only solution, that doesn't work with the fbdev emulation and is probably doing an additional composition to scale down and center their frames through OpenGL.
We might not have a GPU in the system, and we might not even have an entire graphic stack on top either, so I don't think fixing at the user-space level is a good option (especially since we already have an overscan property in DRM).
Hi Maxime, I took a quick look at the first 2 patches in the series and they look good at first glance. I have them in my queue to review more carefully.
Yes, the first one is pretty scary.
If it can ease your review, I made a bunch of unittests to test that code. It's pretty hacky (basically a copy of some kernel structures and the new logic to parse the command line), but it should test it with a significant number of cases:
http://code.bulix.org/4lnlk7-107122?raw
It's pretty straightforward to compile, you just have to link against cmocka.
Can you explain why you can't fix this by specifying a new mode with big porches (as Russell suggested)?
I'll reply to his mail.
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jean-Francois Moine -
Maxime Ripard -
Russell King - ARM Linux -
Sean Paul