| Message ID | 20230704160133.20261-5-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: Improve fbdev emulation for DMA-able framebuffers | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: > Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with > FBINFO_VIRTFB. The framebuffer range is in DMA-able memory and should > be accessed with the CPU's regular memory ops. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/gpu/drm/tegra/fbdev.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/fbdev.c b/drivers/gpu/drm/tegra/fbdev.c > index 82577b7c88da..8074430c52f1 100644 > --- a/drivers/gpu/drm/tegra/fbdev.c > +++ b/drivers/gpu/drm/tegra/fbdev.c > @@ -103,6 +103,8 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, > return PTR_ERR(info); > } > > + info->flags = FBINFO_DEFAULT; > + > fb = tegra_fb_alloc(drm, &cmd, &bo, 1); > if (IS_ERR(fb)) { > err = PTR_ERR(fb); > @@ -132,6 +134,7 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, > } > } > > + info->flags |= FBINFO_VIRTFB; I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c ? I was just curious about the rationale for setting the flags in two steps. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier Am 05.07.23 um 10:34 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with >> FBINFO_VIRTFB. The framebuffer range is in DMA-able memory and should >> be accessed with the CPU's regular memory ops. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Thierry Reding <thierry.reding@gmail.com> >> Cc: Mikko Perttunen <mperttunen@nvidia.com> >> --- >> drivers/gpu/drm/tegra/fbdev.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/tegra/fbdev.c b/drivers/gpu/drm/tegra/fbdev.c >> index 82577b7c88da..8074430c52f1 100644 >> --- a/drivers/gpu/drm/tegra/fbdev.c >> +++ b/drivers/gpu/drm/tegra/fbdev.c >> @@ -103,6 +103,8 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, >> return PTR_ERR(info); >> } >> >> + info->flags = FBINFO_DEFAULT; >> + >> fb = tegra_fb_alloc(drm, &cmd, &bo, 1); >> if (IS_ERR(fb)) { >> err = PTR_ERR(fb); >> @@ -132,6 +134,7 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, >> } >> } >> >> + info->flags |= FBINFO_VIRTFB; > > I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB > > Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c ? > I was just curious about the rationale for setting the flags in two steps. The _DEFAULT flag is really just a zero. And the other flags describe different aspects of the framebuffer. I think it makes sense to set the flags together with the respective state. For example, _VIRTFB is set next to ->screen_buffer, because they belong together. _VIRTFB is currently only used in defio code at https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fb_defio.c#L232 I think the fbdev I/O helpers should also test this flag after all drivers have been annotated correctly. For example, fb_io_read() would WARN_ONCE if the _VIRTFB flag has been set; and fb_sys_read() would warn if it hasn't been set. For the read helpers, it also makes sense to WARN_ONCE if the _READS_FAST flag has not been set. Best regards Thomas > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >
Thomas Zimmermann <tzimmermann@suse.de> writes: [...] >>> + info->flags |= FBINFO_VIRTFB; >> >> I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB >> >> Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c ? >> I was just curious about the rationale for setting the flags in two steps. > > The _DEFAULT flag is really just a zero. And the other flags describe > different aspects of the framebuffer. I think it makes sense to set the > flags together with the respective state. For example, _VIRTFB is set > next to ->screen_buffer, because they belong together. > Yes, that makes sense. > _VIRTFB is currently only used in defio code at > > https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fb_defio.c#L232 > > I think the fbdev I/O helpers should also test this flag after all > drivers have been annotated correctly. For example, fb_io_read() would > WARN_ONCE if the _VIRTFB flag has been set; and fb_sys_read() would warn > if it hasn't been set. For the read helpers, it also makes sense to > WARN_ONCE if the _READS_FAST flag has not been set. > Agreed. Maybe you could add those warn (or at least info or debug?) even if not all drivers have been annotated correctly. That way people can be aware that is missing and fix if there are remaining drivers. > Best regards > Thomas >
Hi Am 05.07.23 um 11:34 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > [...] > >>>> + info->flags |= FBINFO_VIRTFB; >>> >>> I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB >>> >>> Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c ? >>> I was just curious about the rationale for setting the flags in two steps. >> >> The _DEFAULT flag is really just a zero. And the other flags describe >> different aspects of the framebuffer. I think it makes sense to set the >> flags together with the respective state. For example, _VIRTFB is set >> next to ->screen_buffer, because they belong together. >> > > Yes, that makes sense. > >> _VIRTFB is currently only used in defio code at >> >> https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fb_defio.c#L232 >> >> I think the fbdev I/O helpers should also test this flag after all >> drivers have been annotated correctly. For example, fb_io_read() would >> WARN_ONCE if the _VIRTFB flag has been set; and fb_sys_read() would warn >> if it hasn't been set. For the read helpers, it also makes sense to >> WARN_ONCE if the _READS_FAST flag has not been set. >> > > Agreed. Maybe you could add those warn (or at least info or debug?) even > if not all drivers have been annotated correctly. That way people can be > aware that is missing and fix if there are remaining drivers. Yes, we could do that. I want to go through drivers first and fix the low-hanging fruits. Some of the old fbdev drivers use either DMA or I/O memory. They would only be fix-worthy if someone complains. Best regards Thomas > >> Best regards >> Thomas >> >
diff --git a/drivers/gpu/drm/tegra/fbdev.c b/drivers/gpu/drm/tegra/fbdev.c index 82577b7c88da..8074430c52f1 100644 --- a/drivers/gpu/drm/tegra/fbdev.c +++ b/drivers/gpu/drm/tegra/fbdev.c @@ -103,6 +103,8 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, return PTR_ERR(info); } + info->flags = FBINFO_DEFAULT; + fb = tegra_fb_alloc(drm, &cmd, &bo, 1); if (IS_ERR(fb)) { err = PTR_ERR(fb); @@ -132,6 +134,7 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper, } } + info->flags |= FBINFO_VIRTFB; info->screen_base = (void __iomem *)bo->vaddr + offset; info->screen_size = size; info->fix.smem_start = (unsigned long)(bo->iova + offset);
Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with FBINFO_VIRTFB. The framebuffer range is in DMA-able memory and should be accessed with the CPU's regular memory ops. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/gpu/drm/tegra/fbdev.c | 3 +++ 1 file changed, 3 insertions(+)