| Message ID | 20180201130446.6165-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 01, 2018 at 02:04:46PM +0100, Linus Walleij wrote: > The following happened when migrating an old fbdev driver to DRM: > > The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 > or XRGB1555/XBGR1555 i.e. the maximum depth is 15. > > This makes the initialization of the framebuffer fail since > the code in drm_fb_helper_single_fb_probe() assigns the same value > to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes > a 1-to-1 mapping between BPP and depth, which is true in most cases > but typically not for this hardware. > > To support the odd case of a driver supporting 16BPP with only 15 > bits of depth, this patch will make the code loop over the formats > supported on the primary plane and cap the depth to the maximum > supported. > > On the PL110 Integrator, this makes drm_mode_legacy_fb_format() > select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and > thus we get framebuffer, penguin and console on the Integrator/CP. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index e56166334455..5076f9103740 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1720,6 +1720,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > int i; > struct drm_fb_helper_surface_size sizes; > int gamma_size = 0; > + struct drm_plane *plane; > + int best_depth = 0; > > memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); > sizes.surface_depth = 24; > @@ -1727,7 +1729,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > sizes.fb_width = (u32)-1; > sizes.fb_height = (u32)-1; > > - /* if driver picks 8 or 16 by default use that for both depth/bpp */ > + /* > + * If driver picks 8 or 16 by default use that for both depth/bpp > + * to begin with > + */ > if (preferred_bpp != sizes.surface_bpp) > sizes.surface_depth = sizes.surface_bpp = preferred_bpp; > > @@ -1762,6 +1767,39 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > } > } > > + /* > + * If we run into a situation where, for example, the primary plane > + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > + * 16) we need to scale down the depth of the sizes we request. > + */ > + drm_for_each_plane(plane, fb_helper->dev) { > + /* Only check the primary plane */ > + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > + continue; I think this should look at crtc->primary for each of the crtcs managed by the fb_helper. Also this probably shouldn't look at YUV formats at all? I do wonder if instead we should just have the driver specify the pixel format explicitly instead of trying to guess based on bpp? > + > + for (i = 0; i < plane->format_count; i++) { > + const struct drm_format_info *fmt; > + > + fmt = drm_format_info(plane->format_types[i]); > + /* We found a perfect fit, great */ > + if (fmt->depth == sizes.surface_depth) > + break; > + > + /* Skip depths above what we're looking for */ > + if (fmt->depth > sizes.surface_depth) > + continue; > + > + /* Best depth found so far */ > + if (fmt->depth > best_depth) > + best_depth = fmt->depth; > + } > + } > + if (sizes.surface_depth != best_depth) { > + DRM_DEBUG("requested bpp %d, scaled depth down to %d", > + sizes.surface_bpp, best_depth); > + sizes.surface_depth = best_depth; > + } > + > crtc_count = 0; > for (i = 0; i < fb_helper->crtc_count; i++) { > struct drm_display_mode *desired_mode; > -- > 2.14.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 01.02.2018 14.19, skrev Ville Syrjälä: > On Thu, Feb 01, 2018 at 02:04:46PM +0100, Linus Walleij wrote: >> The following happened when migrating an old fbdev driver to DRM: >> >> The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 >> or XRGB1555/XBGR1555 i.e. the maximum depth is 15. >> >> This makes the initialization of the framebuffer fail since >> the code in drm_fb_helper_single_fb_probe() assigns the same value >> to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes >> a 1-to-1 mapping between BPP and depth, which is true in most cases >> but typically not for this hardware. >> >> To support the odd case of a driver supporting 16BPP with only 15 >> bits of depth, this patch will make the code loop over the formats >> supported on the primary plane and cap the depth to the maximum >> supported. >> >> On the PL110 Integrator, this makes drm_mode_legacy_fb_format() >> select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and >> thus we get framebuffer, penguin and console on the Integrator/CP. >> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index e56166334455..5076f9103740 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -1720,6 +1720,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >> int i; >> struct drm_fb_helper_surface_size sizes; >> int gamma_size = 0; >> + struct drm_plane *plane; >> + int best_depth = 0; >> >> memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); >> sizes.surface_depth = 24; >> @@ -1727,7 +1729,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >> sizes.fb_width = (u32)-1; >> sizes.fb_height = (u32)-1; >> >> - /* if driver picks 8 or 16 by default use that for both depth/bpp */ >> + /* >> + * If driver picks 8 or 16 by default use that for both depth/bpp >> + * to begin with >> + */ >> if (preferred_bpp != sizes.surface_bpp) >> sizes.surface_depth = sizes.surface_bpp = preferred_bpp; >> >> @@ -1762,6 +1767,39 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >> } >> } >> >> + /* >> + * If we run into a situation where, for example, the primary plane >> + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth >> + * 16) we need to scale down the depth of the sizes we request. >> + */ >> + drm_for_each_plane(plane, fb_helper->dev) { >> + /* Only check the primary plane */ >> + if (plane->type != DRM_PLANE_TYPE_PRIMARY) >> + continue; > I think this should look at crtc->primary for each of the crtcs managed > by the fb_helper. > > Also this probably shouldn't look at YUV formats at all? > > I do wonder if instead we should just have the driver specify the > pixel format explicitly instead of trying to guess based on bpp? Can drm_mode_config.preferred_depth be used for this? The comment says that it is used for dumb buffers and drm_mode_create_dumb_ioctl() does this: cpp = DIV_ROUND_UP(args->bpp, 8); So it looks like it should be possible to do preferred_depth=15. Noralf. >> + >> + for (i = 0; i < plane->format_count; i++) { >> + const struct drm_format_info *fmt; >> + >> + fmt = drm_format_info(plane->format_types[i]); >> + /* We found a perfect fit, great */ >> + if (fmt->depth == sizes.surface_depth) >> + break; >> + >> + /* Skip depths above what we're looking for */ >> + if (fmt->depth > sizes.surface_depth) >> + continue; >> + >> + /* Best depth found so far */ >> + if (fmt->depth > best_depth) >> + best_depth = fmt->depth; >> + } >> + } >> + if (sizes.surface_depth != best_depth) { >> + DRM_DEBUG("requested bpp %d, scaled depth down to %d", >> + sizes.surface_bpp, best_depth); >> + sizes.surface_depth = best_depth; >> + } >> + >> crtc_count = 0; >> for (i = 0; i < fb_helper->crtc_count; i++) { >> struct drm_display_mode *desired_mode; >> -- >> 2.14.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 01.02.2018 16.15, skrev Noralf Trønnes: > > Den 01.02.2018 14.19, skrev Ville Syrjälä: >> On Thu, Feb 01, 2018 at 02:04:46PM +0100, Linus Walleij wrote: >>> The following happened when migrating an old fbdev driver to DRM: >>> >>> The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 >>> or XRGB1555/XBGR1555 i.e. the maximum depth is 15. >>> >>> This makes the initialization of the framebuffer fail since >>> the code in drm_fb_helper_single_fb_probe() assigns the same value >>> to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes >>> a 1-to-1 mapping between BPP and depth, which is true in most cases >>> but typically not for this hardware. >>> >>> To support the odd case of a driver supporting 16BPP with only 15 >>> bits of depth, this patch will make the code loop over the formats >>> supported on the primary plane and cap the depth to the maximum >>> supported. >>> >>> On the PL110 Integrator, this makes drm_mode_legacy_fb_format() >>> select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and >>> thus we get framebuffer, penguin and console on the Integrator/CP. >>> >>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >>> --- >>> drivers/gpu/drm/drm_fb_helper.c | 40 >>> +++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 39 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c >>> b/drivers/gpu/drm/drm_fb_helper.c >>> index e56166334455..5076f9103740 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -1720,6 +1720,8 @@ static int >>> drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >>> int i; >>> struct drm_fb_helper_surface_size sizes; >>> int gamma_size = 0; >>> + struct drm_plane *plane; >>> + int best_depth = 0; >>> memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); >>> sizes.surface_depth = 24; >>> @@ -1727,7 +1729,10 @@ static int >>> drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >>> sizes.fb_width = (u32)-1; >>> sizes.fb_height = (u32)-1; >>> - /* if driver picks 8 or 16 by default use that for both >>> depth/bpp */ >>> + /* >>> + * If driver picks 8 or 16 by default use that for both depth/bpp >>> + * to begin with >>> + */ >>> if (preferred_bpp != sizes.surface_bpp) >>> sizes.surface_depth = sizes.surface_bpp = preferred_bpp; >>> @@ -1762,6 +1767,39 @@ static int >>> drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, >>> } >>> } >>> + /* >>> + * If we run into a situation where, for example, the primary >>> plane >>> + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, >>> depth >>> + * 16) we need to scale down the depth of the sizes we request. >>> + */ >>> + drm_for_each_plane(plane, fb_helper->dev) { >>> + /* Only check the primary plane */ >>> + if (plane->type != DRM_PLANE_TYPE_PRIMARY) >>> + continue; >> I think this should look at crtc->primary for each of the crtcs managed >> by the fb_helper. >> >> Also this probably shouldn't look at YUV formats at all? >> >> I do wonder if instead we should just have the driver specify the >> pixel format explicitly instead of trying to guess based on bpp? > > Can drm_mode_config.preferred_depth be used for this? The comment says > that > it is used for dumb buffers and drm_mode_create_dumb_ioctl() does this: > cpp = DIV_ROUND_UP(args->bpp, 8); > > So it looks like it should be possible to do preferred_depth=15. > Hmm, maybe not. vc4 sets preferred_depth=24, should that be RGB888 or XRGB8888? The driver prefers 32bpp, but would end up with 24bpp. I'm working on generic fbdev emulation coupled with a in-kernel client api, so I'm interested in a solution to this for the client api. > Noralf. > >>> + >>> + for (i = 0; i < plane->format_count; i++) { >>> + const struct drm_format_info *fmt; >>> + >>> + fmt = drm_format_info(plane->format_types[i]); >>> + /* We found a perfect fit, great */ >>> + if (fmt->depth == sizes.surface_depth) >>> + break; >>> + >>> + /* Skip depths above what we're looking for */ >>> + if (fmt->depth > sizes.surface_depth) >>> + continue; >>> + >>> + /* Best depth found so far */ >>> + if (fmt->depth > best_depth) >>> + best_depth = fmt->depth; >>> + } >>> + } >>> + if (sizes.surface_depth != best_depth) { >>> + DRM_DEBUG("requested bpp %d, scaled depth down to %d", >>> + sizes.surface_bpp, best_depth); >>> + sizes.surface_depth = best_depth; >>> + } >>> + >>> crtc_count = 0; >>> for (i = 0; i < fb_helper->crtc_count; i++) { >>> struct drm_display_mode *desired_mode; >>> -- >>> 2.14.3 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Thu, Feb 1, 2018 at 2:19 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > [Me] >> + /* >> + * If we run into a situation where, for example, the primary plane >> + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth >> + * 16) we need to scale down the depth of the sizes we request. >> + */ >> + drm_for_each_plane(plane, fb_helper->dev) { >> + /* Only check the primary plane */ >> + if (plane->type != DRM_PLANE_TYPE_PRIMARY) >> + continue; > > I think this should look at crtc->primary for each of the crtcs managed > by the fb_helper. > > Also this probably shouldn't look at YUV formats at all? I guess I can look into doing it this way, sorry for not knowing how to properly inspect DRM objects, I'm lost sometimes... > I do wonder if instead we should just have the driver specify the > pixel format explicitly instead of trying to guess based on bpp? That makes a lot more sense to me actually. It would give a better sense of control so the driver feel it knows what is actually going on. So I would just update drm_fb_cma_fbdev_init() and drm_fb_helper_initial_config() to pass a reasonable pixel format instead and refactor all the way down? It does hit a lot of code on the way, but if everyone thinks this is a good idea I can very well take a stab at it. Yours, Linus Walleij
On Fri, Feb 02, 2018 at 02:56:30PM +0100, Linus Walleij wrote: > On Thu, Feb 1, 2018 at 2:19 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > [Me] > >> + /* > >> + * If we run into a situation where, for example, the primary plane > >> + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > >> + * 16) we need to scale down the depth of the sizes we request. > >> + */ > >> + drm_for_each_plane(plane, fb_helper->dev) { > >> + /* Only check the primary plane */ > >> + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > >> + continue; > > > > I think this should look at crtc->primary for each of the crtcs managed > > by the fb_helper. > > > > Also this probably shouldn't look at YUV formats at all? > > I guess I can look into doing it this way, sorry for not knowing how to > properly inspect DRM objects, I'm lost sometimes... > > > I do wonder if instead we should just have the driver specify the > > pixel format explicitly instead of trying to guess based on bpp? > > That makes a lot more sense to me actually. It would > give a better sense of control so the driver feel it knows > what is actually going on. > > So I would just update > drm_fb_cma_fbdev_init() and drm_fb_helper_initial_config() > to pass a reasonable pixel format instead and refactor all the > way down? Yeah, something along those lines would seem like the better approach to me. But it's been a while since I've looked at this code so I might be totally wrong :) > > It does hit a lot of code on the way, but if everyone thinks this > is a good idea I can very well take a stab at it. > > Yours, > Linus Walleij
On Fri, Feb 02, 2018 at 04:03:43PM +0200, Ville Syrjälä wrote: > On Fri, Feb 02, 2018 at 02:56:30PM +0100, Linus Walleij wrote: > > On Thu, Feb 1, 2018 at 2:19 PM, Ville Syrjälä > > <ville.syrjala@linux.intel.com> wrote: > > > [Me] > > >> + /* > > >> + * If we run into a situation where, for example, the primary plane > > >> + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > > >> + * 16) we need to scale down the depth of the sizes we request. > > >> + */ > > >> + drm_for_each_plane(plane, fb_helper->dev) { > > >> + /* Only check the primary plane */ > > >> + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > > >> + continue; > > > > > > I think this should look at crtc->primary for each of the crtcs managed > > > by the fb_helper. > > > > > > Also this probably shouldn't look at YUV formats at all? > > > > I guess I can look into doing it this way, sorry for not knowing how to > > properly inspect DRM objects, I'm lost sometimes... > > > > > I do wonder if instead we should just have the driver specify the > > > pixel format explicitly instead of trying to guess based on bpp? > > > > That makes a lot more sense to me actually. It would > > give a better sense of control so the driver feel it knows > > what is actually going on. > > > > So I would just update > > drm_fb_cma_fbdev_init() and drm_fb_helper_initial_config() > > to pass a reasonable pixel format instead and refactor all the > > way down? > > Yeah, something along those lines would seem like the better approach > to me. But it's been a while since I've looked at this code so I might > be totally wrong :) Yeah, that would definitely fit better with the new world - fbdev emulation hasnt ever been updated to the pixel format approach. But fbdev emulation also predates the list of acceptable formats for each plane, so I'd go one step further and try to out-guess something if the driver passes a pixel format of 0. Your above loop almost does that already. That way only drivers which want something non-standard would need to set that parameter. For everyone else we could go through all rgb/bgr formats, from most pixels to least (well, all those that fbdev supports), with C8 as the ultimate fallback. But just going to pixel formats would be a good step already for sure. -Daniel > > > > > It does hit a lot of code on the way, but if everyone thinks this > > is a good idea I can very well take a stab at it. > > > > Yours, > > Linus Walleij > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e56166334455..5076f9103740 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1720,6 +1720,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int i; struct drm_fb_helper_surface_size sizes; int gamma_size = 0; + struct drm_plane *plane; + int best_depth = 0; memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); sizes.surface_depth = 24; @@ -1727,7 +1729,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, sizes.fb_width = (u32)-1; sizes.fb_height = (u32)-1; - /* if driver picks 8 or 16 by default use that for both depth/bpp */ + /* + * If driver picks 8 or 16 by default use that for both depth/bpp + * to begin with + */ if (preferred_bpp != sizes.surface_bpp) sizes.surface_depth = sizes.surface_bpp = preferred_bpp; @@ -1762,6 +1767,39 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, } } + /* + * If we run into a situation where, for example, the primary plane + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth + * 16) we need to scale down the depth of the sizes we request. + */ + drm_for_each_plane(plane, fb_helper->dev) { + /* Only check the primary plane */ + if (plane->type != DRM_PLANE_TYPE_PRIMARY) + continue; + + for (i = 0; i < plane->format_count; i++) { + const struct drm_format_info *fmt; + + fmt = drm_format_info(plane->format_types[i]); + /* We found a perfect fit, great */ + if (fmt->depth == sizes.surface_depth) + break; + + /* Skip depths above what we're looking for */ + if (fmt->depth > sizes.surface_depth) + continue; + + /* Best depth found so far */ + if (fmt->depth > best_depth) + best_depth = fmt->depth; + } + } + if (sizes.surface_depth != best_depth) { + DRM_DEBUG("requested bpp %d, scaled depth down to %d", + sizes.surface_bpp, best_depth); + sizes.surface_depth = best_depth; + } + crtc_count = 0; for (i = 0; i < fb_helper->crtc_count; i++) { struct drm_display_mode *desired_mode;
The following happened when migrating an old fbdev driver to DRM: The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 or XRGB1555/XBGR1555 i.e. the maximum depth is 15. This makes the initialization of the framebuffer fail since the code in drm_fb_helper_single_fb_probe() assigns the same value to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes a 1-to-1 mapping between BPP and depth, which is true in most cases but typically not for this hardware. To support the odd case of a driver supporting 16BPP with only 15 bits of depth, this patch will make the code loop over the formats supported on the primary plane and cap the depth to the maximum supported. On the PL110 Integrator, this makes drm_mode_legacy_fb_format() select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and thus we get framebuffer, penguin and console on the Integrator/CP. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)