date:20180304

RE: [PATCH] dma-buf/reservation: shouldn't kfree staged when slot available

2018-03-04 Thread Liu, Monk

Hi Christian

You are right on that part of obj-staged is set to NULL in add_fence, 
So my following question will be why we kfree(obj->staged) in reserve_shared() 
if staged is always NULL in that point ?

Thanks 
/Monk

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: 2018年2月28日 16:27
To: Liu, Monk ; dri-devel@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH] dma-buf/reservation: shouldn't kfree staged when slot 
available

Am 28.02.2018 um 07:44 schrieb Monk Liu:
> under below scenario the obj->fence would refer to a wild pointer:
>
> 1,call reservation_object_reserved_shared
> 2,call reservation_object_add_shared_fence
> 3,call reservation_object_reserved_shared
> 4,call reservation_object_add_shared_fence
>
> in step 1, staged is allocated,
>
> in step 2, code path will go reservation_object_add_shared_replace()
> and obj->fence would be assigned as staged (through RCU_INIT_POINTER)
>
> in step 3, obj->staged will be freed(by simple kfree), which make 
> obj->fence point to a wild pointer...


Well that explanation is still nonsense. See
reservation_object_add_shared_fence:
>     obj->staged = NULL;

Among the first things reservation_object_add_shared_fence() does is it 
sets obj->staged to NULL.

So step 3 will not free anything and we never have a wild pointer.

Regards,
Christian.

>
> in step 4, code path will go reservation_object_add_shared_inplace()
> and inside it the @fobj (which equals to @obj->staged, set by above steps)
> is already a wild pointer
>
> should remov the kfree on staged in reservation_object_reserve_shared()
>
> Change-Id: If7c01f1b4be3d3d8a81efa90216841f79ab1fc1c
> Signed-off-by: Monk Liu 
> ---
>   drivers/dma-buf/reservation.c | 7 ++-
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 375de41..b473ccc 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -74,12 +74,9 @@ int reservation_object_reserve_shared(struct 
> reservation_object *obj)
>   old = reservation_object_get_list(obj);
>   
>   if (old && old->shared_max) {
> - if (old->shared_count < old->shared_max) {
> - /* perform an in-place update */
> - kfree(obj->staged);
> - obj->staged = NULL;
> + if (old->shared_count < old->shared_max)
>   return 0;
> - } else
> + else
>   max = old->shared_max * 2;
>   } else
>   max = 4;

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/bridge/synopsys: dw-hdmi: Fix memleak in __dw_hdmi_remove

2018-03-04 Thread Jeffy Chen

The platform_device_register_full() will allocate dma_mask for
hdmi->audio, so we should free before platform_device_unregister().

Reported by kmemleak:
unreferenced object 0xffc0ef70ff00 (size 128):
  comm "kworker/4:1", pid 123, jiffies 4294670080 (age 189.604s)
  hex dump (first 32 bytes):
ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00  
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[<21946f44>] kmemleak_alloc+0x58/0x8c
[<9c43890d>] kmem_cache_alloc_memcg_trace+0x18c/0x25c
[<0e17cd06>] platform_device_register_full+0x64/0x108
[<418a0882>] __dw_hdmi_probe+0xb9c/0xcc0
[] dw_hdmi_bind+0x30/0x88
[<9af347f6>] dw_hdmi_rockchip_bind+0x260/0x2e8

Signed-off-by: Jeffy Chen 
---

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f9802399cc0d..d9afdc59d4f4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2567,8 +2567,10 @@ __dw_hdmi_probe(struct platform_device *pdev,
 
 static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
 {
-   if (hdmi->audio && !IS_ERR(hdmi->audio))
+   if (hdmi->audio && !IS_ERR(hdmi->audio)) {
+   kfree(hdmi->audio->dev.dma_mask);
platform_device_unregister(hdmi->audio);
+   }
if (!IS_ERR(hdmi->cec))
platform_device_unregister(hdmi->cec);
 
-- 
2.11.0


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: move rgb mode_valid from connector to encoder

2018-03-04 Thread Maxime Ripard



On Sun, Mar 04, 2018 at 11:37:50AM +0100, Giulio Benetti wrote:
> mode_valid function must be connected to encoder, not connector.
> Otherwise it doesn't get called by drm.

That's not true, or rather, that's a big oversimplification. It
doesn't get called by DRM if you have a bridge instead of a panel
attached to the encoder.

> Move mode_valid function pointer to encoder helper functions,
> changing its prototype according to encoder helper function pointer.
> 
> Signed-off-by: Giulio Benetti 
> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 102 
> +++---
>  1 file changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c 
> b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index b8da5a5..6539dcc 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -52,11 +52,59 @@ static int sun4i_rgb_get_modes(struct drm_connector 
> *connector)
>   return drm_panel_get_modes(tcon->panel);
>  }
>  
> -static int sun4i_rgb_mode_valid(struct drm_connector *connector,
> - struct drm_display_mode *mode)
> +static struct drm_connector_helper_funcs sun4i_rgb_con_helper_funcs = {
> + .get_modes  = sun4i_rgb_get_modes,
> +};
> +
> +static void
> +sun4i_rgb_connector_destroy(struct drm_connector *connector)
>  {
>   struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector);
>   struct sun4i_tcon *tcon = rgb->tcon;
> +
> + drm_panel_detach(tcon->panel);
> + drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs sun4i_rgb_con_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy= sun4i_rgb_connector_destroy,
> + .reset  = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
> +{
> + struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> + struct sun4i_tcon *tcon = rgb->tcon;
> +
> + DRM_DEBUG_DRIVER("Enabling RGB output\n");
> +
> + if (!IS_ERR(tcon->panel)) {
> + drm_panel_prepare(tcon->panel);
> + drm_panel_enable(tcon->panel);
> + }
> +}
> +
> +static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
> +{
> + struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> + struct sun4i_tcon *tcon = rgb->tcon;
> +
> + DRM_DEBUG_DRIVER("Disabling RGB output\n");
> +
> + if (!IS_ERR(tcon->panel)) {
> + drm_panel_disable(tcon->panel);
> + drm_panel_unprepare(tcon->panel);
> + }
> +}
> +
> +static enum drm_mode_status sun4i_rgb_encoder_mode_valid(struct drm_encoder 
> *crtc,
> +  const struct 
> drm_display_mode *mode)
> +{
> + struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(crtc);
> + struct sun4i_tcon *tcon = rgb->tcon;
>   u32 hsync = mode->hsync_end - mode->hsync_start;
>   u32 vsync = mode->vsync_end - mode->vsync_start;
>   unsigned long rate = mode->clock * 1000;
> @@ -106,58 +154,10 @@ static int sun4i_rgb_mode_valid(struct drm_connector 
> *connector,
>   return MODE_OK;
>  }
>  
> -static struct drm_connector_helper_funcs sun4i_rgb_con_helper_funcs = {
> - .get_modes  = sun4i_rgb_get_modes,
> - .mode_valid = sun4i_rgb_mode_valid,
> -};
> -
> -static void
> -sun4i_rgb_connector_destroy(struct drm_connector *connector)
> -{
> - struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector);
> - struct sun4i_tcon *tcon = rgb->tcon;
> -
> - drm_panel_detach(tcon->panel);
> - drm_connector_cleanup(connector);
> -}
> -
> -static const struct drm_connector_funcs sun4i_rgb_con_funcs = {
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .destroy= sun4i_rgb_connector_destroy,
> - .reset  = drm_atomic_helper_connector_reset,
> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> -};
> -
> -static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
> -{
> - struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> - struct sun4i_tcon *tcon = rgb->tcon;
> -
> - DRM_DEBUG_DRIVER("Enabling RGB output\n");
> -
> - if (!IS_ERR(tcon->panel)) {
> - drm_panel_prepare(tcon->panel);
> - drm_panel_enable(tcon->panel);
> - }
> -}
> -
> -static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
> -{
> - struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> - struct sun4i_tcon *tcon = rgb->tcon;
> -
> - DRM_DEBUG_DRIVER("Disabling RGB output\n");
> -
> - if

Re: nouveau 30bpp / deep color status

2018-03-04 Thread Mario Kleiner

On 02/05/2018 12:50 AM, Ilia Mirkin wrote:

In case anyone's curious about 30bpp framebuffer support, here's the
current status:

Kernel:

Ben and I have switched the code to using a 256-based LUT for Kepler+,
and I've also written a patch to cause the addfb ioctl to use the
proper format. You can pick this up at:

https://github.com/skeggsb/linux/commits/linux-4.16 (note the branch!)
https://patchwork.freedesktop.org/patch/202322/

With these two, you should be able to use "X -depth 30" again on any
G80+ GPU to bring up a screen (as you could in kernel 4.9 and
earlier). However this still has some deficiencies, some of which I've
addressed:

xf86-video-nouveau:

DRI3 was broken, and Xv was broken. Patches available at:

https://github.com/imirkin/xf86-video-nouveau/commits/master

mesa:

The NVIDIA hardware (pre-Kepler) can only do XBGR scanout. Further the
nouveau KMS doesn't add XRGB scanout for Kepler+ (although it could).
Mesa was only enabled for XRGB, so I've piped XBGR through all the
same places:

https://github.com/imirkin/mesa/commits/30bpp

Wrt. mesa, those patches are now in master and i think we have a bit of
a problem under X11+GLX:

https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/dri/dri_screen.c#n108

dri_fill_in_modes() defines MESA_FORMAT_R10G10B10A2_UNORM,
MESA_FORMAT_R10G10B10X2_UNORM at the top inbetween the BGRX/A formats
ignoring the instructions that

"/* The 32-bit RGBA format must not precede the 32-bit BGRA format.
* Likewise for RGBX and BGRX. Otherwise, the GLX client and the GLX
* server may disagree on which format the GLXFBConfig represents,
* resulting in swapped color channels."

RGBA/X formats should only be exposed
if (dri_loader_get_cap(screen, DRI_LOADER_CAP_RGBA_ORDERING))

and that is only the case for the Android loader.

The GLX code doesn't use the red/green/blueChannelMasks for proper
matching of formats, and the server doesn't even transmit those masks to
the client in the case of GLX. So whatever 10 bit format comes first
will win when building the assignment to GLXFBConfigs.

I looked at the code and how it behaves. In practice Intel gfx works
because it's a classic DRI driver with its own method of building the
DRIconfig's, and it only exposes the BGR101010 formats, so no danger of
mixups. AMD's gallium drivers expose both BGR and RGB ordered 10 bit
formats, but due to the ordering, the matching ends up only assigning
the desired BGR formats that are good for AMD hw, discarding the RGB
formats. nouveau works because it only exposes the desired RGB format
for the hw. But with other gallium drivers for some SoC's or future
gallium drivers it is not so clear if the right thing will happen. E.g.,
freedreno seems to support both BGR and RGB 10 bit formats as
PIPE_BIND_DISPLAY_TARGET afaics, so i don't know if by luck the right
thing would happen?

Afaics EGL does the right thing wrt. channelmask matching of EGLConfigs
to DRIconfigs, so we could probably implement dri_loader_get_cap(screen,
DRI_LOADER_CAP_RGBA_ORDERING) == TRUE for the EGL loaders.

But for GLX it is not so easy or quick. I looked if i could make the
servers GLX send proper channelmask attributes and Mesa parsing them,
but there aren't any GLX tags defined for channel masks, and all other
tags come from official GLX extension headers. I'm not sure what the
proper procedure for defining new tags is? Do we have to define a new
GLX extension for that and get it in the Khronos registry and then back
into the server/mesa code-base?

The current patches in mesa for XBGR also lack enablement pieces for
EGL, Wayland and X11 compositing, but that's a different problem.

-mario

libdrm:

For testing, I added a modetest gradient pattern split horizontally.
Top half is 10bpc, bottom half is 8bpc. This is useful for seeing
whether you're really getting 10bpc, or if things are getting
truncated along the way. Definitely hacky, but ... wasn't intending on
upstreaming it anyways:

https://github.com/imirkin/drm/commit/9b8776f58448b5745675c3a7f5eb2735e3989441

Results with the patches (tested on a GK208B and a "deep color" TV over HDMI):
- modetest with a 10bpc gradient shows up smoother than an 8bpc
gradient. However it's still dithered to 8bpc, not "real" 10bpc.
- things generally work in X -- dri2 and dri3, xv, and obviously
regular X rendering / acceleration
- lots of X software can't handle 30bpp modes (mplayer hates it for
xv and x11 rendering, aterm bails on shading the root pixmap, probably
others)

I'm also told that with DP, it should actually send the higher-bpc
data over the wire. With HDMI, we're still stuck at 24bpp for now
(although the hardware can do 36bpp as well). This is why my gradient
result above was still dithered.

Things to do - mostly nouveau specific, but probably some general
infra needed too:
- Figure out how to properly expose the 1024-sized LUT
- Add fp16 scanout
-

35 matches

Mail list logo