| Message ID | 1490348452-20851-27-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 24, 2017 at 11:40:47AM +0200, Tomi Valkeinen wrote: > From: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Add fbdev emulation only for the first DRM connector. > When the fbdev emulation was created for all connectors with different > resolution, the lower res display would only be able to show part of the > framebuffer. > By creating the fbdev emulation only for the first connector we can avoid > this. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Why this driver-specific behavior? This is how it works everywhere. If this doesn't work in some case, then we need to fix this in the fbdev helper. Or have a modparam for that. But definitely not diverging behaviour between drivers. -Daniel > --- > drivers/gpu/drm/omapdrm/omap_fbdev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c > index 73619e201c0c..58c0c5bccc8c 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c > @@ -269,7 +269,9 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) > goto fail; > } > > - ret = drm_fb_helper_single_add_all_connectors(helper); > + drm_modeset_lock_all(dev); > + ret = drm_fb_helper_add_one_connector(helper, priv->connectors[0]); > + drm_modeset_unlock_all(dev); > if (ret) > goto fini; > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 25/03/17 23:22, Daniel Vetter wrote: > On Fri, Mar 24, 2017 at 11:40:47AM +0200, Tomi Valkeinen wrote: >> From: Peter Ujfalusi <peter.ujfalusi@ti.com> >> >> Add fbdev emulation only for the first DRM connector. >> When the fbdev emulation was created for all connectors with different >> resolution, the lower res display would only be able to show part of the >> framebuffer. >> By creating the fbdev emulation only for the first connector we can avoid >> this. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Why this driver-specific behavior? This is how it works everywhere. > > If this doesn't work in some case, then we need to fix this in the fbdev > helper. Or have a modparam for that. But definitely not diverging > behaviour between drivers. The default behavior often results in a rather unusable fbdev on the other screen. For example, a board with a low-res LCD and HDMI. Fbdev is created based in the LCD resolution, and on HDMI you'll get 1080p resolution with a tiny fbdev. Or, if fbdev is created based on the HDMI resolution, on the LCD you'll see a tiny portion of the huge fbdev. I personally did suggest our folks to just disable the fbdev totally, but apparently there are still some uses for the fbdev, so this patch seemed like a simple way to make the behavior a bit nicer. But I agree that it would be best to have this fully configurable, as different use cases have different needs. Then again, I'd rather just disable the fbdev than start spending time on improving it =). Tomi
On Mon, Mar 27, 2017 at 09:28:07AM +0300, Tomi Valkeinen wrote: > On 25/03/17 23:22, Daniel Vetter wrote: > > On Fri, Mar 24, 2017 at 11:40:47AM +0200, Tomi Valkeinen wrote: > >> From: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> > >> Add fbdev emulation only for the first DRM connector. > >> When the fbdev emulation was created for all connectors with different > >> resolution, the lower res display would only be able to show part of the > >> framebuffer. > >> By creating the fbdev emulation only for the first connector we can avoid > >> this. > >> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > Why this driver-specific behavior? This is how it works everywhere. > > > > If this doesn't work in some case, then we need to fix this in the fbdev > > helper. Or have a modparam for that. But definitely not diverging > > behaviour between drivers. > > The default behavior often results in a rather unusable fbdev on the > other screen. > > For example, a board with a low-res LCD and HDMI. Fbdev is created based > in the LCD resolution, and on HDMI you'll get 1080p resolution with a > tiny fbdev. Or, if fbdev is created based on the HDMI resolution, on the > LCD you'll see a tiny portion of the huge fbdev. > > I personally did suggest our folks to just disable the fbdev totally, > but apparently there are still some uses for the fbdev, so this patch > seemed like a simple way to make the behavior a bit nicer. > > But I agree that it would be best to have this fully configurable, as > different use cases have different needs. Then again, I'd rather just > disable the fbdev than start spending time on improving it =). Yeah I think a module option for the fbdev helper which implements exactly this would be fine. Assuming that would make your users happy. I just want to avoid everyone having to hand-roll the same hacks, since this problem is the same everywhere (never boot with a retina screen and the low-res projector plugged in ...). -Daniel
On 27/03/17 09:40, Daniel Vetter wrote: > On Mon, Mar 27, 2017 at 09:28:07AM +0300, Tomi Valkeinen wrote: >> On 25/03/17 23:22, Daniel Vetter wrote: >>> On Fri, Mar 24, 2017 at 11:40:47AM +0200, Tomi Valkeinen wrote: >>>> From: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>> >>>> Add fbdev emulation only for the first DRM connector. >>>> When the fbdev emulation was created for all connectors with different >>>> resolution, the lower res display would only be able to show part of the >>>> framebuffer. >>>> By creating the fbdev emulation only for the first connector we can avoid >>>> this. >>>> >>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >>> >>> Why this driver-specific behavior? This is how it works everywhere. >>> >>> If this doesn't work in some case, then we need to fix this in the fbdev >>> helper. Or have a modparam for that. But definitely not diverging >>> behaviour between drivers. >> >> The default behavior often results in a rather unusable fbdev on the >> other screen. >> >> For example, a board with a low-res LCD and HDMI. Fbdev is created based >> in the LCD resolution, and on HDMI you'll get 1080p resolution with a >> tiny fbdev. Or, if fbdev is created based on the HDMI resolution, on the >> LCD you'll see a tiny portion of the huge fbdev. >> >> I personally did suggest our folks to just disable the fbdev totally, >> but apparently there are still some uses for the fbdev, so this patch >> seemed like a simple way to make the behavior a bit nicer. >> >> But I agree that it would be best to have this fully configurable, as >> different use cases have different needs. Then again, I'd rather just >> disable the fbdev than start spending time on improving it =). > > Yeah I think a module option for the fbdev helper which implements > exactly this would be fine. Assuming that would make your users happy. > > I just want to avoid everyone having to hand-roll the same hacks, since > this problem is the same everywhere (never boot with a retina screen and > the low-res projector plugged in ...). Ok, I'll drop this for now and look for a more generic solution. What should the default behavior be? The current one? Module parameters are often nice, but if the wanted default behavior (as it is for us) requires setting a parameter, it's not so nice. Is it ok if the driver can choose its default behavior, which can then be overridden with module parameters? Then again... I get the gut feeling that this is kind of pointless effort. fbdev won't work well anyway =). Even if we had such a module parameter, would you bother and remember to set it when booting with retina and a low-res projector? When you'd notice that the fbdev is messed up, would you reboot and set the module parameter, or would you just unplug the projector and reboot without it? In other words, would such a parameter really be usable... I don't know... I think the only sane behavior is to have the fbdev only on a single screen. Or, on multiple screens but only if the resolutions of the displays are exactly the same. Or if you can scale the fbdev so that it fits into any screen. Tomi
On Mon, Mar 27, 2017 at 10:25:46AM +0300, Tomi Valkeinen wrote: > On 27/03/17 09:40, Daniel Vetter wrote: > > On Mon, Mar 27, 2017 at 09:28:07AM +0300, Tomi Valkeinen wrote: > >> On 25/03/17 23:22, Daniel Vetter wrote: > >>> On Fri, Mar 24, 2017 at 11:40:47AM +0200, Tomi Valkeinen wrote: > >>>> From: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>>> > >>>> Add fbdev emulation only for the first DRM connector. > >>>> When the fbdev emulation was created for all connectors with different > >>>> resolution, the lower res display would only be able to show part of the > >>>> framebuffer. > >>>> By creating the fbdev emulation only for the first connector we can avoid > >>>> this. > >>>> > >>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >>> > >>> Why this driver-specific behavior? This is how it works everywhere. > >>> > >>> If this doesn't work in some case, then we need to fix this in the fbdev > >>> helper. Or have a modparam for that. But definitely not diverging > >>> behaviour between drivers. > >> > >> The default behavior often results in a rather unusable fbdev on the > >> other screen. > >> > >> For example, a board with a low-res LCD and HDMI. Fbdev is created based > >> in the LCD resolution, and on HDMI you'll get 1080p resolution with a > >> tiny fbdev. Or, if fbdev is created based on the HDMI resolution, on the > >> LCD you'll see a tiny portion of the huge fbdev. > >> > >> I personally did suggest our folks to just disable the fbdev totally, > >> but apparently there are still some uses for the fbdev, so this patch > >> seemed like a simple way to make the behavior a bit nicer. > >> > >> But I agree that it would be best to have this fully configurable, as > >> different use cases have different needs. Then again, I'd rather just > >> disable the fbdev than start spending time on improving it =). > > > > Yeah I think a module option for the fbdev helper which implements > > exactly this would be fine. Assuming that would make your users happy. > > > > I just want to avoid everyone having to hand-roll the same hacks, since > > this problem is the same everywhere (never boot with a retina screen and > > the low-res projector plugged in ...). > > Ok, I'll drop this for now and look for a more generic solution. > > What should the default behavior be? The current one? Module parameters > are often nice, but if the wanted default behavior (as it is for us) > requires setting a parameter, it's not so nice. > > Is it ok if the driver can choose its default behavior, which can then > be overridden with module parameters? > > Then again... I get the gut feeling that this is kind of pointless > effort. fbdev won't work well anyway =). Even if we had such a module > parameter, would you bother and remember to set it when booting with > retina and a low-res projector? When you'd notice that the fbdev is > messed up, would you reboot and set the module parameter, or would you > just unplug the projector and reboot without it? > > In other words, would such a parameter really be usable... > > I don't know... I think the only sane behavior is to have the fbdev only > on a single screen. Or, on multiple screens but only if the resolutions > of the displays are exactly the same. Or if you can scale the fbdev so > that it fits into any screen. I think we could do the module param + Kconfig default like the recent overallocation thing. And yes I'm torn on fbdev too, but otoh if we can make the kms fbdev emulation more feature-full and useful (with minimal amount of work), that means more reasons to abandon fbdev drivers and use kms drivers. Even when userspace won't switch just yet. I think that's good, because it's gets kms foot into the door. Same reason really why we grumpily merged the vblank wait and overallocation/page-flip patches. -Daniel
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 73619e201c0c..58c0c5bccc8c 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -269,7 +269,9 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) goto fail; } - ret = drm_fb_helper_single_add_all_connectors(helper); + drm_modeset_lock_all(dev); + ret = drm_fb_helper_add_one_connector(helper, priv->connectors[0]); + drm_modeset_unlock_all(dev); if (ret) goto fini;