Re: [Intel-gfx] [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".
On Wed, Aug 9, 2017 at 4:13 PM, Joe Kniss wrote:
> On Wed, Aug 9, 2017 at 12:14 PM, Noralf Trønnes wrote:
>>
>> Den 09.08.2017 01.42, skrev Joe Kniss:
>>>
>>> Because all drivers currently use gem objects for framebuffer planes,
>>> the virtual create_handle() is not required. This change adds a
>>> struct drm_gem_object *gems[4] field to drm_framebuffer and removes
>>> create_handle() function pointer from drm_framebuffer_funcs. The
>>> corresponding *_create_handle() function is removed from each driver.
>>>
>>> In many cases this change eliminates a struct *_framebuffer object,
>>> as the only need for the derived struct is the addition of the gem
>>> object pointer.
>>>
>>> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915, rockchip
>>>
>>> Signed-off-by: Joe Kniss
>>> ---
>>
>>
>> Hi Joe,
>>
>> I'm also looking into adding gem objs to drm_framebuffer in this patch:
>> [PATCH v2 01/22] drm: Add GEM backed framebuffer library
>> https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
>>
>
> Great. There's only minimal overlap here. I'll rebase this change on yours
> once it's in.
>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>>> index ade319d10e70..f5f011b910b1 100644
>>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>> @@ -31,14 +31,9 @@
>>> #define DEFAULT_FBDEFIO_DELAY_MS 50
>>> -struct drm_fb_cma {
>>> - struct drm_framebuffer fb;
>>> - struct drm_gem_cma_object *obj[4];
>>> -};
>>> -
>>> struct drm_fbdev_cma {
>>> struct drm_fb_helperfb_helper;
>>> - struct drm_fb_cma *fb;
>>> + struct drm_framebuffer *fb;
>>
>>
>> This fb pointer isn't necessary, since fb_helper already has one.
>>
So, looking deeper into this, it seems that the struct
drm_framebuffer_funcs *fb_funcs is also redundant here? In which case
this whole struct can go...
>
> I'll remove it... but I am sure when I look deeper there will be more
> of these in the various drivers too.
>
>> Noralf.
>>
>>
>
> -joe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".
On Wed, Aug 9, 2017 at 12:14 PM, Noralf Trønnes wrote:
>
> Den 09.08.2017 01.42, skrev Joe Kniss:
>>
>> Because all drivers currently use gem objects for framebuffer planes,
>> the virtual create_handle() is not required. This change adds a
>> struct drm_gem_object *gems[4] field to drm_framebuffer and removes
>> create_handle() function pointer from drm_framebuffer_funcs. The
>> corresponding *_create_handle() function is removed from each driver.
>>
>> In many cases this change eliminates a struct *_framebuffer object,
>> as the only need for the derived struct is the addition of the gem
>> object pointer.
>>
>> TESTED: compiled: allyesconfig ARCH=x86,arm platforms:i915, rockchip
>>
>> Signed-off-by: Joe Kniss
>> ---
>
>
> Hi Joe,
>
> I'm also looking into adding gem objs to drm_framebuffer in this patch:
> [PATCH v2 01/22] drm: Add GEM backed framebuffer library
> https://lists.freedesktop.org/archives/dri-devel/2017-August/149782.html
>
Great. There's only minimal overlap here. I'll rebase this change on yours
once it's in.
> [...]
>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>> index ade319d10e70..f5f011b910b1 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -31,14 +31,9 @@
>> #define DEFAULT_FBDEFIO_DELAY_MS 50
>> -struct drm_fb_cma {
>> - struct drm_framebuffer fb;
>> - struct drm_gem_cma_object *obj[4];
>> -};
>> -
>> struct drm_fbdev_cma {
>> struct drm_fb_helperfb_helper;
>> - struct drm_fb_cma *fb;
>> + struct drm_framebuffer *fb;
>
>
> This fb pointer isn't necessary, since fb_helper already has one.
>
I'll remove it... but I am sure when I look deeper there will be more
of these in the various drivers too.
> Noralf.
>
>
-joe
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".
I didn't mean to add libdrm, sorry, there is a separate change for that. I'll double check and test the drivers that don't implement the virtual. Thanks for the quick reply! -j On Tue, Aug 8, 2017 at 4:50 PM, Dave Airlie wrote: > On 9 August 2017 at 09:42, Joe Kniss wrote: > > Because all drivers currently use gem objects for framebuffer planes, > > the virtual create_handle() is not required. This change adds a > > struct drm_gem_object *gems[4] field to drm_framebuffer and removes > > create_handle() function pointer from drm_framebuffer_funcs. The > > corresponding *_create_handle() function is removed from each driver. > > > > In many cases this change eliminates a struct *_framebuffer object, > > as the only need for the derived struct is the addition of the gem > > object pointer. > > Why the libdrm in the tag? this isn't for libdrm. > > This will break drivers that don't current implement the virtual at all. > > I think vwmgfx. > > The current code checks if the virtual is there before callng it, this code > just calls the gem code always. > > Dave. > -- Dr. Joe Michael Kniss | Google ChromeOS | d...@google.com | 1-801-898-7977 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.
On Tue, Aug 1, 2017 at 1:46 PM, Laurent Pinchart < laurent.pinch...@ideasonboard.com> wrote: > Hi Joe, > > On Tuesday 01 Aug 2017 10:24:25 Joe Kniss wrote: > > On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart wrote: > > > On Monday 31 Jul 2017 11:29:13 Joe Kniss wrote: > > >> New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric > > >> with addfb2. > > > > > > What's the use case for this ? We haven't needed such an ioctl for so > long > > > that it seemed to me that userspace doesn't really need it, but I > could be > > > wrong. > > > > Sorry, I failed to reference the original email. Here it is: > > > > > > I am a recent addition to Google's ChromeOS gfx team. I am currently > > working on display testing and reporting. An important part of this is > > our screen capture tool, which works by querying drm for crtcs, planes, > and > > fbs. Unfortunately, there is only limited information available via > > drmModeGetFB(), which often wrong information when drmModeAddFB2() was > used > > to create the fbs. For example, if the pixel format is NV12 or YUV420, > > the fb returned knows nothing about the additional buffer planes required > > by these formats. Ideally, we would like a function (e.g. > drmModeGetFB2) > > to return information symmetric with drmModeAddFB2 including the pixel > > format id, buffer plane information etc. > > > > > > ChromeOS has needed this functionality from the start, for both testing > and > > error reporting. We got away with guessing the buffer's format (32bit > > xrgb) until now. We are now enabling overlays and more formats including > > multi-planar (e.g. NV12). Current getfb() reports neither the pixel > format > > nor planar information. Without this information, going forward, our > gfx > > testing is going to break. It would be great if we had access to higher > > level buffer structs (like gbm), but we generally don't since they would > be > > created by other apps (chrome browser, android apps, etc...). > > How is screen capture implemented ? Do you enumerate the framebuffers being > scanned out, dump their contents and compose them manually based on the > active > plane configuration ? If so, isn't this very race-prone ? > > Yes, this is basically it. We identify the crtc to capture, query the planes associated with it and their properties. For each plane, we get the fb, then a FD that we use to import a GBM buffer, which we map and composite. It's not ideal, but it's the only thing that will work across all of our platforms and configs. We either wait or sleep for consistent testing captures. I'm not sure what we can do about the race condition without a much more substantial change... We are currently looking into some platform specific methods, but the current approach won't be going away anytime soon. > > >> Also modifies *_fb_create_handle() calls to accept a > > >> format_plane_index so that handles for each plane can be generated. > > >> Previously, many *_fb_create_handle() calls simply defaulted to plane > 0 > > >> only. > > > > > > And with this patch the amd/amdgpu, armada, gma500, i915, mediatek, > msm, > > > nouveau and radeon drivers still do. Do none of them support > multi-planar > > > formats ? > > > > I would imagine that some of these do support multi-planar formats, but > > they don't appear to have them implemented yet (except perhaps as offsets > > into a single buffer). I will certainly be looking into this soon, but > any > > changes will come in future patches. > > > > >> Signed-off-by: Joe Kniss > > >> --- > > >> > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +- > > >> drivers/gpu/drm/armada/armada_fb.c | 1 + > > >> drivers/gpu/drm/drm_crtc_internal.h | 2 + > > >> drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++-- > > >> drivers/gpu/drm/drm_framebuffer.c | 79 > +++- > > >> drivers/gpu/drm/drm_ioctl.c | 1 + > > >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 7 ++- > > >> drivers/gpu/drm/gma500/framebuffer.c| 2 + > > >> drivers/gpu/drm/i915/intel_display.c| 1 + > > >> drivers/gpu/drm/mediatek/mtk_drm_fb.c | 1 + > > >> drivers/gpu/drm/msm/msm_fb.c| 5 +- > > >> drivers/gpu/drm/nouveau/nouveau_display.c | 1 + > >
Re: [Intel-gfx] [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.
Thanks for the quick review!
On Tue, Aug 1, 2017 at 5:09 AM, Laurent Pinchart wrote:
> Hi Joe,
>
> Thank you for the patch.
>
> On Monday 31 Jul 2017 11:29:13 Joe Kniss wrote:
> > New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
> > with addfb2.
>
> What's the use case for this ? We haven't needed such an ioctl for so long
> that it seemed to me that userspace doesn't really need it, but I could be
> wrong.
>
> Sorry, I failed to reference the original email. Here it is:
I am a recent addition to Google's ChromeOS gfx team. I am currently
working on display testing and reporting. An important part of this is
our screen capture tool, which works by querying drm for crtcs, planes, and
fbs. Unfortunately, there is only limited information available via
drmModeGetFB(), which often wrong information when drmModeAddFB2() was used
to create the fbs. For example, if the pixel format is NV12 or YUV420,
the fb returned knows nothing about the additional buffer planes required
by these formats. Ideally, we would like a function (e.g. drmModeGetFB2)
to return information symmetric with drmModeAddFB2 including the pixel
format id, buffer plane information etc.
ChromeOS has needed this functionality from the start, for both testing and
error reporting. We got away with guessing the buffer's format (32bit
xrgb) until now. We are now enabling overlays and more formats including
multi-planar (e.g. NV12). Current getfb() reports neither the pixel format
nor planar information. Without this information, going forward, our gfx
testing is going to break. It would be great if we had access to higher
level buffer structs (like gbm), but we generally don't since they would be
created by other apps (chrome browser, android apps, etc...).
> > Also modifies *_fb_create_handle() calls to accept a
> > format_plane_index so that handles for each plane can be generated.
> > Previously, many *_fb_create_handle() calls simply defaulted to plane 0
> > only.
>
> And with this patch the amd/amdgpu, armada, gma500, i915, mediatek, msm,
> nouveau and radeon drivers still do. Do none of them support multi-planar
> formats ?
>
> I would imagine that some of these do support multi-planar formats, but
they don't appear to have them implemented yet (except perhaps as offsets
into a single buffer). I will certainly be looking into this soon, but any
changes will come in future patches.
> > Signed-off-by: Joe Kniss
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +-
> > drivers/gpu/drm/armada/armada_fb.c | 1 +
> > drivers/gpu/drm/drm_crtc_internal.h | 2 +
> > drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++--
> > drivers/gpu/drm/drm_framebuffer.c | 79
> +-
> > drivers/gpu/drm/drm_ioctl.c | 1 +
> > drivers/gpu/drm/exynos/exynos_drm_fb.c | 7 ++-
> > drivers/gpu/drm/gma500/framebuffer.c| 2 +
> > drivers/gpu/drm/i915/intel_display.c| 1 +
> > drivers/gpu/drm/mediatek/mtk_drm_fb.c | 1 +
> > drivers/gpu/drm/msm/msm_fb.c| 5 +-
> > drivers/gpu/drm/nouveau/nouveau_display.c | 1 +
> > drivers/gpu/drm/omapdrm/omap_fb.c | 5 +-
> > drivers/gpu/drm/radeon/radeon_display.c | 5 +-
> > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++-
> > drivers/gpu/drm/tegra/fb.c | 9 +++-
> > include/drm/drm_framebuffer.h | 1 +
> > include/uapi/drm/drm.h | 2 +
> > 18 files changed, 127 insertions(+), 17 deletions(-)
>
> [snip]
>
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c index 28a0108a1ab8..67b3be1bedbc
> 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "drm_crtc_internal.h"
>
> [snip]
>
> > +/**
> > + * drm_mode_getfb2 - get FB info
> > + * @dev: drm device for the ioctl
> > + * @data: data pointer for the ioctl
> > + * @file_priv: drm file for the ioctl call
> > + *
> > + * Lookup the FB given its ID and return info about it.
> > + *
> > + * Called by the user via ioctl.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_mode_getfb2(struct drm_device *dev,
> > +void *data, struct drm_file *file_priv)
> > +{
> > + struct drm_mode_fb_cmd2 *r = data;
> > + struct drm_framebuffer *fb;
> > +
[Intel-gfx] [PATCH] Add drm ioctl DRM_IOCTL_MODE_GETFB2 & associated helpers.
New getfb2 functionality uses drm_mode_fb_cmd2 struct to be symmetric
with addfb2. Also modifies *_fb_create_handle() calls to accept a
format_plane_index so that handles for each plane can be generated.
Previously, many *_fb_create_handle() calls simply defaulted to plane 0 only.
Signed-off-by: Joe Kniss
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +-
drivers/gpu/drm/armada/armada_fb.c | 1 +
drivers/gpu/drm/drm_crtc_internal.h | 2 +
drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++--
drivers/gpu/drm/drm_framebuffer.c | 79 -
drivers/gpu/drm/drm_ioctl.c | 1 +
drivers/gpu/drm/exynos/exynos_drm_fb.c | 7 ++-
drivers/gpu/drm/gma500/framebuffer.c| 2 +
drivers/gpu/drm/i915/intel_display.c| 1 +
drivers/gpu/drm/mediatek/mtk_drm_fb.c | 1 +
drivers/gpu/drm/msm/msm_fb.c| 5 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 1 +
drivers/gpu/drm/omapdrm/omap_fb.c | 5 +-
drivers/gpu/drm/radeon/radeon_display.c | 5 +-
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 6 ++-
drivers/gpu/drm/tegra/fb.c | 9 +++-
include/drm/drm_framebuffer.h | 1 +
include/uapi/drm/drm.h | 2 +
18 files changed, 127 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 39fc388f222a..c77c1cd265a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -566,8 +566,9 @@ static void amdgpu_user_framebuffer_destroy(struct
drm_framebuffer *fb)
}
static int amdgpu_user_framebuffer_create_handle(struct drm_framebuffer *fb,
- struct drm_file *file_priv,
- unsigned int *handle)
+unsigned int plane_index,
+struct drm_file *file_priv,
+unsigned int *handle)
{
struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
diff --git a/drivers/gpu/drm/armada/armada_fb.c
b/drivers/gpu/drm/armada/armada_fb.c
index 2a7eb6817c36..9f237544f6c5 100644
--- a/drivers/gpu/drm/armada/armada_fb.c
+++ b/drivers/gpu/drm/armada/armada_fb.c
@@ -23,6 +23,7 @@ static void armada_fb_destroy(struct drm_framebuffer *fb)
}
static int armada_fb_create_handle(struct drm_framebuffer *fb,
+ unsigned int format_plane_index,
struct drm_file *dfile, unsigned int *handle)
{
struct armada_framebuffer *dfb = drm_fb_to_armada_fb(fb);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index 955c5690bf64..ec8d913240fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -170,6 +170,8 @@ int drm_mode_rmfb(struct drm_device *dev,
void *data, struct drm_file *file_priv);
int drm_mode_getfb(struct drm_device *dev,
void *data, struct drm_file *file_priv);
+int drm_mode_getfb2(struct drm_device *dev,
+ void *data, struct drm_file *file_priv);
int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv);
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
b/drivers/gpu/drm/drm_fb_cma_helper.c
index 596fabf18c3e..5fd7bcc2c6d1 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -110,13 +110,16 @@ void drm_fb_cma_destroy(struct drm_framebuffer *fb)
}
EXPORT_SYMBOL(drm_fb_cma_destroy);
-int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
- struct drm_file *file_priv, unsigned int *handle)
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+ unsigned int format_plane_index,
+ struct drm_file *file_priv,
+ unsigned int *handle)
{
struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
+ if (format_plane_index >= 4 || !fb_dma->obj[format_plane_index])
+ return -ENOENT;
return drm_gem_handle_create(file_priv,
- &fb_cma->obj[0]->base, handle);
+ &fb_cma->obj[format_plane_index]->base, handle);
}
EXPORT_SYMBOL(drm_fb_cma_create_handle);
diff --git a/drivers/gpu/drm/drm_framebuffer.c
b/drivers/gpu/drm/drm_framebuffer.c
index 28a0108a1ab8..67b3be1bedbc 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include "drm_crtc_internal.h"
@@ -438,7 +439,7 @@ int drm_mode_getfb(struct drm_device *dev,
if (fb->funcs->create_handle) {
if (drm_is_current_
