| Message ID | 1501601201-32590-1-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: > The fbdev subsystem has a place for physical dimensions (width and height > in mm) that is readable by userspace. Since DRM also knows these > dimensions, pass this information to the fbdev device. > > Signed-off-by: David Lechner <david@lechnology.com> Still in the wrong function. Also please add some notation about what you changed when resubmitting a patch (it took me a while to remember that I replied to you already). That makes patch reviewing more efficient. Thanks, Daniel > --- > drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 574af01..07a6621 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1759,6 +1759,7 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe > uint32_t fb_width, uint32_t fb_height) > { > struct drm_framebuffer *fb = fb_helper->fb; > + int i; > > info->pseudo_palette = fb_helper->pseudo_palette; > info->var.xres_virtual = fb->width; > @@ -1771,6 +1772,18 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe > info->var.height = -1; > info->var.width = -1; > > + drm_fb_helper_for_each_connector(fb_helper, i) { > + struct drm_connector *connector = > + fb_helper->connector_info[i]->connector; > + > + /* use the first connected connector for the physical dimensions */ > + if (connector->status == connector_status_connected) { > + info->var.height = connector->display_info.width_mm; > + info->var.width = connector->display_info.height_mm; > + break; > + } > + } > + > switch (fb->format->depth) { > case 8: > info->var.red.offset = 0; > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 08/02/2017 04:46 AM, Daniel Vetter wrote: > On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: >> The fbdev subsystem has a place for physical dimensions (width and height >> in mm) that is readable by userspace. Since DRM also knows these >> dimensions, pass this information to the fbdev device. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Still in the wrong function. Also please add some notation about what you > changed when resubmitting a patch (it took me a while to remember that I > replied to you already). That makes patch reviewing more efficient. > Sorry for being so dense. :-/ I did read your first reply at least 10 times. All of the terminology is foreign to me, but after sleeping on it a few days, I think it is slowly soaking into my brain.
On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote: > On 08/02/2017 04:46 AM, Daniel Vetter wrote: >> >> On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: >>> >>> The fbdev subsystem has a place for physical dimensions (width and height >>> in mm) that is readable by userspace. Since DRM also knows these >>> dimensions, pass this information to the fbdev device. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >> >> >> Still in the wrong function. Also please add some notation about what you >> changed when resubmitting a patch (it took me a while to remember that I >> replied to you already). That makes patch reviewing more efficient. >> > > Sorry for being so dense. :-/ > > I did read your first reply at least 10 times. All of the terminology is > foreign to me, but after sleeping on it a few days, I think it is slowly > soaking into my brain. No problem, the code is fairly convoluted. One more question on your v3: From reading fbdev code I don't see any place that overwrites the physical dimensions except the fill_var helper function (which is called deep down from register_framebuffer). If we entirely remove the var.width/height assignments from that (including the -1 default) and move all of it into setup_crtcs, would that work? I kinda don't like have the same logic in 2 completely different places, once for driver load and once for hotplug handling. That tends to cause bugs (because then no one bothers to test hotplug handling or the boot-up case properly). Thanks, Daniel
On 08/02/2017 06:28 PM, Daniel Vetter wrote: > On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote: >> On 08/02/2017 04:46 AM, Daniel Vetter wrote: >>> >>> On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: >>>> >>>> The fbdev subsystem has a place for physical dimensions (width and height >>>> in mm) that is readable by userspace. Since DRM also knows these >>>> dimensions, pass this information to the fbdev device. >>>> >>>> Signed-off-by: David Lechner <david@lechnology.com> >>> >>> >>> Still in the wrong function. Also please add some notation about what you >>> changed when resubmitting a patch (it took me a while to remember that I >>> replied to you already). That makes patch reviewing more efficient. >>> >> >> Sorry for being so dense. :-/ >> >> I did read your first reply at least 10 times. All of the terminology is >> foreign to me, but after sleeping on it a few days, I think it is slowly >> soaking into my brain. > > No problem, the code is fairly convoluted. One more question on your > v3: From reading fbdev code I don't see any place that overwrites the > physical dimensions except the fill_var helper function (which is > called deep down from register_framebuffer). If we entirely remove the > var.width/height assignments from that (including the -1 default) and > move all of it into setup_crtcs, would that work? I tried that, but in my case, drm_setup_crtcs() is only called once before the fbdev is allocated. So, I got a kernel oops from dereferencing a null pointer, which is why there is a null check for fb_helper->fbdev as part of the if statement I added there. Since drm_setup_crtcs() is not called again, the var.width and height are never set. I also noticed that there is another similar workaround/code duplication for the framebuffer not being allocated in the first call to drm_setup_crtcs() seen at the end of drm_fb_helper_single_fb_probe(). > > I kinda don't like have the same logic in 2 completely different > places, once for driver load and once for hotplug handling. That tends > to cause bugs (because then no one bothers to test hotplug handling or > the boot-up case properly). > Would it be possible to split drm_setup_crtcs() into two functions, a top half and a bottom half. During init (pseudo-code)... drm_begin_setup_crtcs() drm_fb_helper_single_fb_probe() drm_finish_setup_crtcs() and during hotplug event, just... drm_begin_setup_crtcs() drm_finish_crtcs_bottom_half() I think it could solve both cases.
On Wed, Aug 02, 2017 at 08:37:56PM -0500, David Lechner wrote: > On 08/02/2017 06:28 PM, Daniel Vetter wrote: > > On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote: > > > On 08/02/2017 04:46 AM, Daniel Vetter wrote: > > > > > > > > On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: > > > > > > > > > > The fbdev subsystem has a place for physical dimensions (width and height > > > > > in mm) that is readable by userspace. Since DRM also knows these > > > > > dimensions, pass this information to the fbdev device. > > > > > > > > > > Signed-off-by: David Lechner <david@lechnology.com> > > > > > > > > > > > > Still in the wrong function. Also please add some notation about what you > > > > changed when resubmitting a patch (it took me a while to remember that I > > > > replied to you already). That makes patch reviewing more efficient. > > > > > > > > > > Sorry for being so dense. :-/ > > > > > > I did read your first reply at least 10 times. All of the terminology is > > > foreign to me, but after sleeping on it a few days, I think it is slowly > > > soaking into my brain. > > > > No problem, the code is fairly convoluted. One more question on your > > v3: From reading fbdev code I don't see any place that overwrites the > > physical dimensions except the fill_var helper function (which is > > called deep down from register_framebuffer). If we entirely remove the > > var.width/height assignments from that (including the -1 default) and > > move all of it into setup_crtcs, would that work? > > > I tried that, but in my case, drm_setup_crtcs() is only called once before > the fbdev is allocated. So, I got a kernel oops from dereferencing a null > pointer, which is why there is a null check for fb_helper->fbdev as part of > the if statement I added there. Since drm_setup_crtcs() is not called again, > the var.width and height are never set. fb_helper->fbdev is allocated in drm_fb_helper_alloc_fbi, which /should/ be called as the very first thing in setting up the entire fbdev stuff. If you're oopsing on that, something went wrong. We also set info->var in a few other places before calling register_framebuffer already (see the line that sets info->var.pixclock right before calling register_framebuffer). > I also noticed that there is another similar workaround/code duplication for > the framebuffer not being allocated in the first call to drm_setup_crtcs() > seen at the end of drm_fb_helper_single_fb_probe(). > > > > > I kinda don't like have the same logic in 2 completely different > > places, once for driver load and once for hotplug handling. That tends > > to cause bugs (because then no one bothers to test hotplug handling or > > the boot-up case properly). > > > > Would it be possible to split drm_setup_crtcs() into two functions, a top > half and a bottom half. During init (pseudo-code)... > > drm_begin_setup_crtcs() > drm_fb_helper_single_fb_probe() > drm_finish_setup_crtcs() > > and during hotplug event, just... > > drm_begin_setup_crtcs() > drm_finish_crtcs_bottom_half() > > I think it could solve both cases. I'm still not following where exactly and how you're blowing up. Can pls paste the backtrace of how that all blows up? Thanks, Daniel
On Thu, Aug 3, 2017 at 12:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Aug 02, 2017 at 08:37:56PM -0500, David Lechner wrote: >> On 08/02/2017 06:28 PM, Daniel Vetter wrote: >> > On Wed, Aug 2, 2017 at 6:37 PM, David Lechner <david@lechnology.com> wrote: >> > > On 08/02/2017 04:46 AM, Daniel Vetter wrote: >> > > > >> > > > On Tue, Aug 01, 2017 at 10:26:41AM -0500, David Lechner wrote: >> > > > > >> > > > > The fbdev subsystem has a place for physical dimensions (width and height >> > > > > in mm) that is readable by userspace. Since DRM also knows these >> > > > > dimensions, pass this information to the fbdev device. >> > > > > >> > > > > Signed-off-by: David Lechner <david@lechnology.com> >> > > > >> > > > >> > > > Still in the wrong function. Also please add some notation about what you >> > > > changed when resubmitting a patch (it took me a while to remember that I >> > > > replied to you already). That makes patch reviewing more efficient. >> > > > >> > > >> > > Sorry for being so dense. :-/ >> > > >> > > I did read your first reply at least 10 times. All of the terminology is >> > > foreign to me, but after sleeping on it a few days, I think it is slowly >> > > soaking into my brain. >> > >> > No problem, the code is fairly convoluted. One more question on your >> > v3: From reading fbdev code I don't see any place that overwrites the >> > physical dimensions except the fill_var helper function (which is >> > called deep down from register_framebuffer). If we entirely remove the >> > var.width/height assignments from that (including the -1 default) and >> > move all of it into setup_crtcs, would that work? >> >> >> I tried that, but in my case, drm_setup_crtcs() is only called once before >> the fbdev is allocated. So, I got a kernel oops from dereferencing a null >> pointer, which is why there is a null check for fb_helper->fbdev as part of >> the if statement I added there. Since drm_setup_crtcs() is not called again, >> the var.width and height are never set. > > fb_helper->fbdev is allocated in drm_fb_helper_alloc_fbi, which /should/ > be called as the very first thing in setting up the entire fbdev stuff. > If you're oopsing on that, something went wrong. We also set info->var in > a few other places before calling register_framebuffer already (see the > line that sets info->var.pixclock right before calling > register_framebuffer). > >> I also noticed that there is another similar workaround/code duplication for >> the framebuffer not being allocated in the first call to drm_setup_crtcs() >> seen at the end of drm_fb_helper_single_fb_probe(). Ok I screwed up, alloc_fbi is called from fb_probe, which is called only from helper_single_probe(). Sorry about all my confusion, sounds like we indeed have to duplicate this. Since the duplication is necessary, can you pls extract an update_var static helper function, which is called from both places? I'd put the call in drm_fb_helper_hotplugt_event and right before the register_framebuffer for the load path, not trying to be clever like I suggested and hiding it somewhere in drm_setup_crtcs. Sorry for going all over the place with this, but fbdev isn't really something I know all that well ... -Daniel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 574af01..07a6621 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1759,6 +1759,7 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe uint32_t fb_width, uint32_t fb_height) { struct drm_framebuffer *fb = fb_helper->fb; + int i; info->pseudo_palette = fb_helper->pseudo_palette; info->var.xres_virtual = fb->width; @@ -1771,6 +1772,18 @@ void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helpe info->var.height = -1; info->var.width = -1; + drm_fb_helper_for_each_connector(fb_helper, i) { + struct drm_connector *connector = + fb_helper->connector_info[i]->connector; + + /* use the first connected connector for the physical dimensions */ + if (connector->status == connector_status_connected) { + info->var.height = connector->display_info.width_mm; + info->var.width = connector->display_info.height_mm; + break; + } + } + switch (fb->format->depth) { case 8: info->var.red.offset = 0;
The fbdev subsystem has a place for physical dimensions (width and height in mm) that is readable by userspace. Since DRM also knows these dimensions, pass this information to the fbdev device. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/gpu/drm/drm_fb_helper.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)