Re: [PATCH v2 00/14] drm/format-helper: Move to struct iosys_map

[Noralf Trønnes]

Den 08.08.2022 14.53, skrev Thomas Zimmermann:
> Change format-conversion helpers to use struct iosys_map for source
> and destination buffers. Update all users. Also prepare interface for
> multi-plane color formats.
> 
> The format-conversion helpers mostly used to convert to I/O memory
> or system memory. To actual memory type depended on the usecase. We
> now have drivers upcomming that do the conversion entirely in system
> memory. It's a good opportunity to stream-line the interface of the
> conversion helpers to use struct iosys_map. Source and destination
> buffers can now be either in system or in I/O memory. Note that the
> implementation still only supports source buffers in system memory.
> 
> This patchset also changes the interface to support multi-plane
> color formats, where the values for each component are stored in
> distinct memory locations. Converting from RGBRGBRGB to RRRGGGBBB
> would require a single source buffer with RGB values and 3 destination
> buffers for the R, G and B values. Conversion-helper interfaces now
> support this.
> 
> v2:
>   * add IOSYS_MAP_INIT_VADDR_IOMEM (Sam)
>   * use drm_format_info_bpp() (Sam)
>   * update documentation (Sam)
>   * rename 'vmap' to 'src' (Sam)
>   * many smaller cleanups and fixes (Sam, Jose)
> 
> Thomas Zimmermann (14):
>   iosys-map: Add IOSYS_MAP_INIT_VADDR_IOMEM()
>   drm/format-helper: Provide drm_fb_blit()
>   drm/format-helper: Merge drm_fb_memcpy() and drm_fb_memcpy_toio()
>   drm/format-helper: Convert drm_fb_swab() to struct iosys_map
>   drm/format-helper: Rework XRGB-to-RGBG332 conversion
>   drm/format-helper: Rework XRGB-to-RGBG565 conversion
>   drm/format-helper: Rework XRGB-to-RGB888 conversion
>   drm/format-helper: Rework RGB565-to-XRGB conversion
>   drm/format-helper: Rework RGB888-to-XRGB conversion
>   drm/format-helper: Rework XRGB-to-XRGB2101010 conversion
>   drm/format-helper: Rework XRGB-to-GRAY8 conversion
>   drm/format-helper: Rework XRGB-to-MONO conversion
>   drm/format-helper: Move destination-buffer handling into internal
> helper
>   drm/format-helper: Rename parameter vmap to src
> 

Tested-by: Noralf Trønnes 

* gud: XRGB-to-{MONO,GRAY8,RGB332,RGB565}
* mi0283qt (drm_mipi_dbi): XRGB-to-RGB565 with swap=true,
drm_fb_memcpy, drm_fb_swab

>  drivers/gpu/drm/drm_format_helper.c   | 509 ++
>  drivers/gpu/drm/drm_mipi_dbi.c|   9 +-
>  drivers/gpu/drm/gud/gud_pipe.c|  20 +-
>  drivers/gpu/drm/hyperv/hyperv_drm_modeset.c   |   9 +-
>  drivers/gpu/drm/mgag200/mgag200_mode.c|   9 +-
>  drivers/gpu/drm/solomon/ssd130x.c |   7 +-
>  .../gpu/drm/tests/drm_format_helper_test.c|  45 +-
>  drivers/gpu/drm/tiny/cirrus.c |  19 +-
>  drivers/gpu/drm/tiny/repaper.c|   6 +-
>  drivers/gpu/drm/tiny/simpledrm.c  |   8 +-
>  drivers/gpu/drm/tiny/st7586.c |   5 +-
>  include/drm/drm_format_helper.h   |  56 +-
>  include/linux/iosys-map.h |  15 +-
>  13 files changed, 416 insertions(+), 301 deletions(-)
> 
> 
> base-commit: 2bdae66c9988dd0f07633629c0a85383cfc05940
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 10.11.2021 11.37, skrev Thomas Zimmermann:
> Enable the FB_DAMAGE_CLIPS property to reduce display-update
> overhead. Also fixes a warning in the kernel log.
> 
>   simple-framebuffer simple-framebuffer.0: [drm] 
> drm_plane_enable_fb_damage_clips() not called
> 
> Fix the computation of the blit rectangle. This wasn't an issue so
> far, as simpledrm always blitted the full framebuffer. The code now
> supports damage clipping and virtual screen sizes.
> 
> v3:
>   * fix drm_dev_enter() error path (Noralf)
>   * remove unnecessary clipping from update function (Noralf)
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 09.11.2021 15.56, skrev Thomas Zimmermann:
> Hi,
> 
> thanks for looking through all this code.
> 
> Am 09.11.21 um 14:04 schrieb Noralf Trønnes:
>>
>>
>> Den 09.11.2021 13.38, skrev Thomas Zimmermann:
>>>
>>>
>>> Am 08.11.21 um 21:55 schrieb Noralf Trønnes:
>>>>
>>>>
>>>> Den 01.11.2021 15.15, skrev Thomas Zimmermann:
>>>>> Enable the FB_DAMAGE_CLIPS property to reduce display-update
>>>>> overhead. Also fixes a warning in the kernel log.
>>>>>
>>>>>     simple-framebuffer simple-framebuffer.0: [drm]
>>>>> drm_plane_enable_fb_damage_clips() not called
>>>>>
>>>>> Fix the computation of the blit rectangle. This wasn't an issue so
>>>>> far, as simpledrm always blitted the full framebuffer. The code now
>>>>> supports damage clipping and virtual screen sizes.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann 
>>>>> ---
>>>>>    drivers/gpu/drm/tiny/simpledrm.c | 30
>>>>> ++
>>>>>    1 file changed, 22 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>>>> index 571f716ff427..e872121e9fb0 100644
>>>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>>>> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct
>>>>> drm_simple_display_pipe *pipe,
>>>>>    void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use
>>>>> mapping abstraction */
>>>>>    struct drm_device *dev = >dev;
>>>>>    void __iomem *dst = sdev->screen_base;
>>>>> -    struct drm_rect clip;
>>>>> +    struct drm_rect src_clip, dst_clip;
>>>>>    int idx;
>>>>>      if (!fb)
>>>>> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct
>>>>> drm_simple_display_pipe *pipe,
>>>>>    if (!drm_dev_enter(dev, ))
>>>>>    return;
>>>>>    -    drm_rect_init(, 0, 0, fb->width, fb->height);
>>>>> +    drm_rect_fp_to_int(_clip, _state->src);
>>>>>    -    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
>>>>> -    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
>>>>> fb, );
>>>>> +    dst_clip = plane_state->dst;
>>>>
>>>> I assume that src_clip and dst_clip are of the same size, since scaling
>>>> is not supported. What prevents dst_clip from being bigger than the
>>>> buffer that's being blitted into? Where is that bounds checking done?
>>>
>>> The value of dst_clip comes from plane_state->dst, which gets
>>> initialized in drm_atomic_helper_check_plane_state(). [1] The fields are
>>> taken from the crtc_{x,y,w,h} values by drm_plane_get_dest(). [2] For
>>> primary planes, the crtc_{x,y,w,h} values are initialized in
>>> __drm_atomic_helper_set_config() to the size of the display. [3] That
>>> size comes directly from the current video mode. [4] From all I can see
>>> this cannot overflow.
>>>
>>
>> Ok, that takes care of the upper bound.
>>
>> There's this in drm_atomic_helper_check_plane_state():
>>
>> plane_state->visible = drm_rect_clip_scaled(src, dst, );
>>
>> if (!plane_state->visible)
>>     /*
>>  * Plane isn't visible; some drivers can handle this
>>  * so we just return success here.  Drivers that can't
>>  * (including those that use the primary plane helper's
>>  * update function) will return an error from their
>>  * update_plane handler.
>>  */
>>     return 0;
>>
>> drm_atomic_helper_damage_merged() checks ->visible and returns false if
>> it is not set so update() is good on the lower bound.
>>
>> Maybe it's necessary to check ->visible here in enable() before doing
>> the blit?
> 
> Is it even possible to create an invisible primary plane here? We cannot
> scale [1] and if the primary plane is smaller than the framebuffer, we
> hit the case at [2]. The only way I can see this is that the visibility
> test at [3] succeeds. That would require a framebuffer of size 0. That's
> impossible from the code in frambuffer_check(). [4] Unl

Re: [PATCH v2 8/9] drm/simpledrm: Support virtual screen sizes

[Noralf Trønnes]

Den 09.11.2021 10.06, skrev Thomas Zimmermann:
> Hi
> 
> Am 08.11.21 um 22:01 schrieb Noralf Trønnes:
>>
>>
>> Den 01.11.2021 15.15, skrev Thomas Zimmermann:
>>> Add constants for the maximum size of the shadow-plane surface
>>> size. Useful for shadow planes with virtual screen sizes. The
>>> current sizes are 4096 scanlines with 4096 pixels each. This
>>> seems reasonable for current hardware, but can be increased as
>>> necessary.
>>>
>>> In simpledrm, set the maximum framebuffer size from the constants
>>> for shadow planes. Implements support for virtual screen sizes and
>>> page flipping on the fbdev console.
>>>
>>> Signed-off-by: Thomas Zimmermann 
>>> ---
>>>   drivers/gpu/drm/tiny/simpledrm.c    |  9 +++--
>>>   include/drm/drm_gem_atomic_helper.h | 18 ++
>>>   2 files changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>> index e872121e9fb0..e42ae1c6ebcd 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -2,6 +2,7 @@
>>>     #include 
>>>   #include 
>>> +#include 
>>>   #include 
>>>   #include 
>>>   #include 
>>> @@ -776,6 +777,7 @@ static int simpledrm_device_init_modeset(struct
>>> simpledrm_device *sdev)
>>>   struct drm_display_mode *mode = >mode;
>>>   struct drm_connector *connector = >connector;
>>>   struct drm_simple_display_pipe *pipe = >pipe;
>>> +    unsigned long max_width, max_height;
>>>   const uint32_t *formats;
>>>   size_t nformats;
>>>   int ret;
>>> @@ -784,10 +786,13 @@ static int simpledrm_device_init_modeset(struct
>>> simpledrm_device *sdev)
>>>   if (ret)
>>>   return ret;
>>>   +    max_width = max_t(unsigned long, mode->hdisplay,
>>> DRM_SHADOW_PLANE_MAX_WIDTH);
>>> +    max_height = max_t(unsigned long, mode->vdisplay,
>>> DRM_SHADOW_PLANE_MAX_HEIGHT);
>>> +
>>>   dev->mode_config.min_width = mode->hdisplay;
>>> -    dev->mode_config.max_width = mode->hdisplay;
>>> +    dev->mode_config.max_width = max_width;
>>>   dev->mode_config.min_height = mode->vdisplay;
>>> -    dev->mode_config.max_height = mode->vdisplay;
>>> +    dev->mode_config.max_height = max_height;
>>>   dev->mode_config.prefer_shadow_fbdev = true;
>>>   dev->mode_config.preferred_depth = sdev->format->cpp[0] * 8;
>>>   dev->mode_config.funcs = _mode_config_funcs;
>>> diff --git a/include/drm/drm_gem_atomic_helper.h
>>> b/include/drm/drm_gem_atomic_helper.h
>>> index 48222a107873..54983ecf641a 100644
>>> --- a/include/drm/drm_gem_atomic_helper.h
>>> +++ b/include/drm/drm_gem_atomic_helper.h
>>> @@ -22,6 +22,24 @@ int drm_gem_simple_display_pipe_prepare_fb(struct
>>> drm_simple_display_pipe *pipe,
>>>    * Helpers for planes with shadow buffers
>>>    */
>>>   +/**
>>> + * DRM_SHADOW_PLANE_MAX_WIDTH - Maximum width of a plane's shadow
>>> buffer in pixels
>>> + *
>>> + * For drivers with shadow planes, the maximum width of the
>>> framebuffer is
>>> + * usually independent from hardware limitations. Drivers can
>>> initialize struct
>>> + * drm_mode_config.max_width from DRM_SHADOW_PLANE_MAX_WIDTH.
>>
>> Why would a driver do that instead of using a value of its own? Is it
>> some kind of standardization?
> 
> Exactly. The shadow framebuffer is in system memory, so its size is
> arbitrarily large. If each driver sets its own limit, it just fragments
> the DRM feature set. There's usually no reason why one driver can have
> 4096 pixels and another one just 2048 or even 8192. Setting a constant
> harmonizes this among drivers.
> 
> Please note that nothing really depends on this value. Drivers can still
> use a different limit if they have to.
> 
>>
>>> + */
>>> +#define DRM_SHADOW_PLANE_MAX_WIDTH    (1ul << 12)
>>
>> Please use a decimal number, I'm so slow at doing this in my head that I
>> use bash to calculate it for me, which really slows down parsing the
>> code.
> 
> Ok. :D
> 

With that fixed:

Acked-by: Noralf Trønnes 


> Best regard
> Thomas
> 
>>
>> Noralf.
>>
>>> +
>>> +/**
>>> + * DRM_SHADOW_PLANE_MAX_HEIGHT - Maximum height of a plane's shadow
>>> buffer in scanlines
>>> + *
>>> + * For drivers with shadow planes, the maximum height of the
>>> framebuffer is
>>> + * usually independent from hardware limitations. Drivers can
>>> initialize struct
>>> + * drm_mode_config.max_height from DRM_SHADOW_PLANE_MAX_HEIGHT.
>>> + */
>>> +#define DRM_SHADOW_PLANE_MAX_HEIGHT    (1ul << 12)
>>> +
>>>   /**
>>>    * struct drm_shadow_plane_state - plane state for planes with
>>> shadow buffers
>>>    *
>>>
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 09.11.2021 13.38, skrev Thomas Zimmermann:
> 
> 
> Am 08.11.21 um 21:55 schrieb Noralf Trønnes:
>>
>>
>> Den 01.11.2021 15.15, skrev Thomas Zimmermann:
>>> Enable the FB_DAMAGE_CLIPS property to reduce display-update
>>> overhead. Also fixes a warning in the kernel log.
>>>
>>>    simple-framebuffer simple-framebuffer.0: [drm]
>>> drm_plane_enable_fb_damage_clips() not called
>>>
>>> Fix the computation of the blit rectangle. This wasn't an issue so
>>> far, as simpledrm always blitted the full framebuffer. The code now
>>> supports damage clipping and virtual screen sizes.
>>>
>>> Signed-off-by: Thomas Zimmermann 
>>> ---
>>>   drivers/gpu/drm/tiny/simpledrm.c | 30 ++
>>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 571f716ff427..e872121e9fb0 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct
>>> drm_simple_display_pipe *pipe,
>>>   void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use
>>> mapping abstraction */
>>>   struct drm_device *dev = >dev;
>>>   void __iomem *dst = sdev->screen_base;
>>> -    struct drm_rect clip;
>>> +    struct drm_rect src_clip, dst_clip;
>>>   int idx;
>>>     if (!fb)
>>> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct
>>> drm_simple_display_pipe *pipe,
>>>   if (!drm_dev_enter(dev, ))
>>>   return;
>>>   -    drm_rect_init(, 0, 0, fb->width, fb->height);
>>> +    drm_rect_fp_to_int(_clip, _state->src);
>>>   -    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
>>> -    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
>>> fb, );
>>> +    dst_clip = plane_state->dst;
>>
>> I assume that src_clip and dst_clip are of the same size, since scaling
>> is not supported. What prevents dst_clip from being bigger than the
>> buffer that's being blitted into? Where is that bounds checking done?
> 
> The value of dst_clip comes from plane_state->dst, which gets
> initialized in drm_atomic_helper_check_plane_state(). [1] The fields are
> taken from the crtc_{x,y,w,h} values by drm_plane_get_dest(). [2] For
> primary planes, the crtc_{x,y,w,h} values are initialized in
> __drm_atomic_helper_set_config() to the size of the display. [3] That
> size comes directly from the current video mode. [4] From all I can see
> this cannot overflow.
> 

Ok, that takes care of the upper bound.

There's this in drm_atomic_helper_check_plane_state():

plane_state->visible = drm_rect_clip_scaled(src, dst, );

if (!plane_state->visible)
/*
 * Plane isn't visible; some drivers can handle this
 * so we just return success here.  Drivers that can't
 * (including those that use the primary plane helper's
 * update function) will return an error from their
 * update_plane handler.
 */
return 0;

drm_atomic_helper_damage_merged() checks ->visible and returns false if
it is not set so update() is good on the lower bound.

Maybe it's necessary to check ->visible here in enable() before doing
the blit?

Noralf.

> Best regards
> Thomas
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L807
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/include/drm/drm_plane.h#L257
> [3]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic.c#L1590
> 
> [4]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_modes.c#L788
> 
> 
>>
>> Noralf.
>>
>>> +    if (!drm_rect_intersect(_clip, _clip))
>>> +    return;
>>> +
>>> +    dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
>>> +    drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap,
>>> fb, _clip);
>>>     drm_dev_exit(idx);
>>>   }
>>> @@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>>   struct drm_framebuffer *fb = plane_state->fb;
>>>   struct drm_device *dev = >dev;
>>>   void __iomem *dst = sdev->screen_base;
>>> 

Re: [PATCH v2 9/9] drm: Clarify semantics of struct drm_mode_config.{min,max}_{width,height}

[Noralf Trønnes]

Den 01.11.2021 15.15, skrev Thomas Zimmermann:
> Add additional information on the semantics of the size fields in
> struct drm_mode_config. Also add a TODO to review all driver for
> correct usage of these fields.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 8/9] drm/simpledrm: Support virtual screen sizes

[Noralf Trønnes]

Den 01.11.2021 15.15, skrev Thomas Zimmermann:
> Add constants for the maximum size of the shadow-plane surface
> size. Useful for shadow planes with virtual screen sizes. The
> current sizes are 4096 scanlines with 4096 pixels each. This
> seems reasonable for current hardware, but can be increased as
> necessary.
> 
> In simpledrm, set the maximum framebuffer size from the constants
> for shadow planes. Implements support for virtual screen sizes and
> page flipping on the fbdev console.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/tiny/simpledrm.c|  9 +++--
>  include/drm/drm_gem_atomic_helper.h | 18 ++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
> b/drivers/gpu/drm/tiny/simpledrm.c
> index e872121e9fb0..e42ae1c6ebcd 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -776,6 +777,7 @@ static int simpledrm_device_init_modeset(struct 
> simpledrm_device *sdev)
>   struct drm_display_mode *mode = >mode;
>   struct drm_connector *connector = >connector;
>   struct drm_simple_display_pipe *pipe = >pipe;
> + unsigned long max_width, max_height;
>   const uint32_t *formats;
>   size_t nformats;
>   int ret;
> @@ -784,10 +786,13 @@ static int simpledrm_device_init_modeset(struct 
> simpledrm_device *sdev)
>   if (ret)
>   return ret;
>  
> + max_width = max_t(unsigned long, mode->hdisplay, 
> DRM_SHADOW_PLANE_MAX_WIDTH);
> + max_height = max_t(unsigned long, mode->vdisplay, 
> DRM_SHADOW_PLANE_MAX_HEIGHT);
> +
>   dev->mode_config.min_width = mode->hdisplay;
> - dev->mode_config.max_width = mode->hdisplay;
> + dev->mode_config.max_width = max_width;
>   dev->mode_config.min_height = mode->vdisplay;
> - dev->mode_config.max_height = mode->vdisplay;
> + dev->mode_config.max_height = max_height;
>   dev->mode_config.prefer_shadow_fbdev = true;
>   dev->mode_config.preferred_depth = sdev->format->cpp[0] * 8;
>   dev->mode_config.funcs = _mode_config_funcs;
> diff --git a/include/drm/drm_gem_atomic_helper.h 
> b/include/drm/drm_gem_atomic_helper.h
> index 48222a107873..54983ecf641a 100644
> --- a/include/drm/drm_gem_atomic_helper.h
> +++ b/include/drm/drm_gem_atomic_helper.h
> @@ -22,6 +22,24 @@ int drm_gem_simple_display_pipe_prepare_fb(struct 
> drm_simple_display_pipe *pipe,
>   * Helpers for planes with shadow buffers
>   */
>  
> +/**
> + * DRM_SHADOW_PLANE_MAX_WIDTH - Maximum width of a plane's shadow buffer in 
> pixels
> + *
> + * For drivers with shadow planes, the maximum width of the framebuffer is
> + * usually independent from hardware limitations. Drivers can initialize 
> struct
> + * drm_mode_config.max_width from DRM_SHADOW_PLANE_MAX_WIDTH.

Why would a driver do that instead of using a value of its own? Is it
some kind of standardization?

> + */
> +#define DRM_SHADOW_PLANE_MAX_WIDTH   (1ul << 12)

Please use a decimal number, I'm so slow at doing this in my head that I
use bash to calculate it for me, which really slows down parsing the code.

Noralf.

> +
> +/**
> + * DRM_SHADOW_PLANE_MAX_HEIGHT - Maximum height of a plane's shadow buffer 
> in scanlines
> + *
> + * For drivers with shadow planes, the maximum height of the framebuffer is
> + * usually independent from hardware limitations. Drivers can initialize 
> struct
> + * drm_mode_config.max_height from DRM_SHADOW_PLANE_MAX_HEIGHT.
> + */
> +#define DRM_SHADOW_PLANE_MAX_HEIGHT  (1ul << 12)
> +
>  /**
>   * struct drm_shadow_plane_state - plane state for planes with shadow buffers
>   *
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 01.11.2021 15.15, skrev Thomas Zimmermann:
> Enable the FB_DAMAGE_CLIPS property to reduce display-update
> overhead. Also fixes a warning in the kernel log.
> 
>   simple-framebuffer simple-framebuffer.0: [drm] 
> drm_plane_enable_fb_damage_clips() not called
> 
> Fix the computation of the blit rectangle. This wasn't an issue so
> far, as simpledrm always blitted the full framebuffer. The code now
> supports damage clipping and virtual screen sizes.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
> b/drivers/gpu/drm/tiny/simpledrm.c
> index 571f716ff427..e872121e9fb0 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping 
> abstraction */
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - drm_rect_init(, 0, 0, fb->width, fb->height);
> + drm_rect_fp_to_int(_clip, _state->src);
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst_clip = plane_state->dst;

I assume that src_clip and dst_clip are of the same size, since scaling
is not supported. What prevents dst_clip from being bigger than the
buffer that's being blitted into? Where is that bounds checking done?

Noralf.

> + if (!drm_rect_intersect(_clip, _clip))
> + return;
> +
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct 
> drm_simple_display_pipe *pipe,
>   struct drm_framebuffer *fb = plane_state->fb;
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect damage_clip, src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
>   return;
>  
> - if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> ))
> + if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> _clip))
> + return;
> +
> + drm_rect_fp_to_int(_clip, _state->src);
> + if (!drm_rect_intersect(_clip, _clip))
> + return;
> +
> + dst_clip = plane_state->dst;
> + if (!drm_rect_intersect(_clip, _clip))
>   return;
>  
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -794,6 +806,8 @@ static int simpledrm_device_init_modeset(struct 
> simpledrm_device *sdev)
>   if (ret)
>   return ret;
>  
> + drm_plane_enable_fb_damage_clips(>plane);
> +
>   drm_mode_config_reset(dev);
>  
>   return 0;
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 01.11.2021 15.15, skrev Thomas Zimmermann:
> Enable the FB_DAMAGE_CLIPS property to reduce display-update
> overhead. Also fixes a warning in the kernel log.
> 
>   simple-framebuffer simple-framebuffer.0: [drm] 
> drm_plane_enable_fb_damage_clips() not called
> 
> Fix the computation of the blit rectangle. This wasn't an issue so
> far, as simpledrm always blitted the full framebuffer. The code now
> supports damage clipping and virtual screen sizes.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
> b/drivers/gpu/drm/tiny/simpledrm.c
> index 571f716ff427..e872121e9fb0 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping 
> abstraction */
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - drm_rect_init(, 0, 0, fb->width, fb->height);
> + drm_rect_fp_to_int(_clip, _state->src);
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst_clip = plane_state->dst;
> + if (!drm_rect_intersect(_clip, _clip))
> + return;

You're inside drm_dev_enter here so can't just return. Move
drm_dev_enter after this like you do in update().

> +
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct 
> drm_simple_display_pipe *pipe,
>   struct drm_framebuffer *fb = plane_state->fb;
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect damage_clip, src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
>   return;
>  
> - if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> ))
> + if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> _clip))
> + return;
> +

The following check, isn't that the same check that has just happened in
drm_atomic_helper_damage_iter_next()?

Noralf.

> + drm_rect_fp_to_int(_clip, _state->src);
> + if (!drm_rect_intersect(_clip, _clip))
> + return;
> +
> + dst_clip = plane_state->dst;
> + if (!drm_rect_intersect(_clip, _clip))
>   return;
>  
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -794,6 +806,8 @@ static int simpledrm_device_init_modeset(struct 
> simpledrm_device *sdev)
>   if (ret)
>   return ret;
>  
> + drm_plane_enable_fb_damage_clips(>plane);
> +
>   drm_mode_config_reset(dev);
>  
>   return 0;
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/9] drm/format-helper: Export drm_fb_clip_offset()

[Noralf Trønnes]

Den 01.11.2021 15.15, skrev Thomas Zimmermann:
> Provide a function that computes the offset into a blit destination
> buffer. This will allow to move destination-buffer clipping into the
> format-helper callers.
> 
> v2:
>   * provide documentation (Sam)
>   * return 'unsigned int' (Sam, Noralf)
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 7/9] drm/simpledrm: Enable FB_DAMAGE_CLIPS property

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Enable the FB_DAMAGE_CLIPS property to reduce display-update
> overhead. Also fixes a warning in the kernel log.
> 
>   simple-framebuffer simple-framebuffer.0: [drm] 
> drm_plane_enable_fb_damage_clips() not called
> 
> Fix the computation of the blit rectangle. This wasn't an issue so
> far, as simpledrm always blitted the full framebuffer. The code now
> supports damage clipping and virtual screen sizes.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c 
> b/drivers/gpu/drm/tiny/simpledrm.c
> index 571f716ff427..e872121e9fb0 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -642,7 +642,7 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   void *vmap = shadow_plane_state->data[0].vaddr; /* TODO: Use mapping 
> abstraction */
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
> @@ -651,10 +651,14 @@ simpledrm_simple_display_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - drm_rect_init(, 0, 0, fb->width, fb->height);
> + drm_rect_fp_to_int(_clip, _state->src);
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst_clip = plane_state->dst;
> + if (!drm_rect_intersect(_clip, _clip))
> + return;
> +
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -686,20 +690,28 @@ simpledrm_simple_display_pipe_update(struct 
> drm_simple_display_pipe *pipe,
>   struct drm_framebuffer *fb = plane_state->fb;
>   struct drm_device *dev = >dev;
>   void __iomem *dst = sdev->screen_base;
> - struct drm_rect clip;
> + struct drm_rect damage_clip, src_clip, dst_clip;
>   int idx;
>  
>   if (!fb)
>   return;
>  
> - if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> ))
> + if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, 
> _clip))
> + return;
> +

I'm afraid I don't understand what's going on here, but isn't it
possible to put this logic into drm_atomic_helper_damage_merged()?

Noralf.

> + drm_rect_fp_to_int(_clip, _state->src);
> + if (!drm_rect_intersect(_clip, _clip))
> + return;
> +
> + dst_clip = plane_state->dst;
> + if (!drm_rect_intersect(_clip, _clip))
>   return;
>  
>   if (!drm_dev_enter(dev, ))
>   return;
>  
> - dst += drm_fb_clip_offset(sdev->pitch, sdev->format, );
> - drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> );
> + dst += drm_fb_clip_offset(sdev->pitch, sdev->format, _clip);
> + drm_fb_blit_toio(dst, sdev->pitch, sdev->format->format, vmap, fb, 
> _clip);
>  
>   drm_dev_exit(idx);
>  }
> @@ -794,6 +806,8 @@ static int simpledrm_device_init_modeset(struct 
> simpledrm_device *sdev)
>   if (ret)
>   return ret;
>  
> + drm_plane_enable_fb_damage_clips(>plane);
> +
>   drm_mode_config_reset(dev);
>  
>   return 0;
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 6/9] drm/fb-helper: Allocate shadow buffer of surface height

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Allocating a shadow buffer of the height of the buffer object does
> not support fbdev overallocation. Use surface height instead.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 5/9] drm/format-helper: Streamline blit-helper interface

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Move destination-buffer clipping from format-helper blit function into
> caller. Rename drm_fb_blit_rect_dstclip() to drm_fb_blit_toio(). Done for
> consistency with the rest of the interface. Remove drm_fb_blit_dstclip(),
> which isn't required.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/9] drm/format-helper: Rework format-helper conversion functions

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Move destination-buffer clipping from all format-helper conversion
> functions into callers. Support destination-buffer pitch. Only
> distinguish between system and I/O memory, but use same logic
> everywhere.
> 
> Simply harmonize the interface and semantics of the existing code.
> Not all conversion helpers support all combinations of parameters.
> We have to add additional features when we need them.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

>  /**
>   * drm_fb_xrgb_to_gray8 - Convert XRGB to grayscale
>   * @dst: 8-bit grayscale destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
>   * @vaddr: XRGB source buffer
>   * @fb: DRM framebuffer
>   * @clip: Clip rectangle area to copy
> @@ -415,16 +417,21 @@ EXPORT_SYMBOL(drm_fb_xrgb_to_rgb888_dstclip);
>   *
>   * ITU BT.601 is used for the RGB -> luma (brightness) conversion.
>   */
> -void drm_fb_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer 
> *fb,
> -struct drm_rect *clip)
> +void drm_fb_xrgb_to_gray8(void *dst, unsigned int dst_pitch, const void 
> *vaddr,
> +   const struct drm_framebuffer *fb, const struct 
> drm_rect *clip)
>  {
>   unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
>   unsigned int x, y;
>   void *buf;
> - u32 *src;
> + u8 *dst8;
> + u32 *src32;
>  
>   if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB))
>   return;
> +
> + if (!dst_pitch)

len is source length (should really have been named src_len) which
results in a kernel crash:

> + dst_pitch = len;

This works:

dst_pitch = drm_rect_width(clip);

With that fixed:

Tested-by: Noralf Trønnes 
Reviewed-by: Noralf Trønnes 

> +
>   /*
>* The cma memory is write-combined so reads are uncached.
>* Speed up by fetching one line at a time.
> @@ -433,20 +440,22 @@ void drm_fb_xrgb_to_gray8(u8 *dst, void *vaddr, 
> struct drm_framebuffer *fb,
>   if (!buf)
>   return;
>  
> + vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
>   for (y = clip->y1; y < clip->y2; y++) {
> - src = vaddr + (y * fb->pitches[0]);
> - src += clip->x1;
> - memcpy(buf, src, len);
> - src = buf;
> + dst8 = dst;
> + src32 = memcpy(buf, vaddr, len);
>   for (x = clip->x1; x < clip->x2; x++) {
> - u8 r = (*src & 0x00ff) >> 16;
> - u8 g = (*src & 0xff00) >> 8;
> - u8 b =  *src & 0x00ff;
> + u8 r = (*src32 & 0x00ff) >> 16;
> + u8 g = (*src32 & 0xff00) >> 8;
> + u8 b =  *src32 & 0x00ff;
>  
>   /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> - *dst++ = (3 * r + 6 * g + b) / 10;
> - src++;
> + *dst8++ = (3 * r + 6 * g + b) / 10;
> + src32++;
>   }
> +
> + vaddr += fb->pitches[0];
> + dst += dst_pitch;
>   }
>  
>   kfree(buf);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/9] drm/format-helper: Add destination-buffer pitch to drm_fb_swab()

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Add destination-buffer pitch as argument to drm_fb_swab(). Done for
> consistency with the rest of the interface.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_format_helper.c | 19 +++
>  drivers/gpu/drm/drm_mipi_dbi.c  |  2 +-
>  drivers/gpu/drm/gud/gud_pipe.c  |  2 +-
>  include/drm/drm_format_helper.h |  5 +++--
>  4 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c 
> b/drivers/gpu/drm/drm_format_helper.c
> index 38c8055f6fa8..79869ed553d9 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -92,6 +92,7 @@ EXPORT_SYMBOL(drm_fb_memcpy_toio);
>  /**
>   * drm_fb_swab - Swap bytes into clip buffer
>   * @dst: Destination buffer
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
>   * @src: Source buffer
>   * @fb: DRM framebuffer
>   * @clip: Clip rectangle area to copy
> @@ -103,19 +104,25 @@ EXPORT_SYMBOL(drm_fb_memcpy_toio);
>   * This function does not apply clipping on dst, i.e. the destination

You have changed this line on the other functions, maybe you just missed
it here:

>   * is a small buffer containing the clip rect only.
>   */
> -void drm_fb_swab(void *dst, void *src, struct drm_framebuffer *fb,
> -  struct drm_rect *clip, bool cached)
> +void drm_fb_swab(void *dst, unsigned int dst_pitch, const void *src,
> +  const struct drm_framebuffer *fb, const struct drm_rect *clip,
> +  bool cached)

Tested-by: Noralf Trønnes 
Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/9] drm/format-helper: Rework format-helper memcpy functions

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Move destination-buffer clipping from all format-helper memcpy
> function into callers. Support destination-buffer pitch. Only
> distinguish between system and I/O memory, but use same logic
> everywhere.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Tested-by: Noralf Trønnes 
Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/9] drm/format-helper: Export drm_fb_clip_offset()

[Noralf Trønnes]

Den 22.10.2021 15.28, skrev Thomas Zimmermann:
> Provide a function that computes the offset into a blit destination
> buffer. This will allow to move destination-buffer clipping into the
> format-helper callers.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_format_helper.c | 10 --
>  include/drm/drm_format_helper.h |  4 
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c 
> b/drivers/gpu/drm/drm_format_helper.c
> index 69fde60e36b3..28e9d0d89270 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -17,12 +17,18 @@
>  #include 
>  #include 
>  
> -static unsigned int clip_offset(struct drm_rect *clip,
> - unsigned int pitch, unsigned int cpp)
> +static unsigned int clip_offset(const struct drm_rect *clip, unsigned int 
> pitch, unsigned int cpp)
>  {
>   return clip->y1 * pitch + clip->x1 * cpp;
>  }
>  
> +unsigned long drm_fb_clip_offset(unsigned int pitch, const struct 
> drm_format_info *format,

Like Sam I wonder about the unsigned long here.

Noralf.

> +  const struct drm_rect *clip)
> +{
> + return clip_offset(clip, pitch, format->cpp[0]);
> +}
> +EXPORT_SYMBOL(drm_fb_clip_offset);
> +

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 39/44] drm/cirrus: Use devm_drm_dev_alloc

[Noralf Trønnes]

Den 03.04.2020 15.58, skrev Daniel Vetter:
> Already using devm_drm_dev_init, so very simple replacment.
> 
> Signed-off-by: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: Gerd Hoffmann 
> Cc: Daniel Vetter 
> Cc: Sam Ravnborg 
> Cc: "Noralf Trønnes" 
> Cc: Rob Herring 
> Cc: Thomas Zimmermann 
> Cc: virtualization@lists.linux-foundation.org
> Cc: Emil Velikov 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] drm/qxl: add drm_driver.release callback.

[Noralf Trønnes]

(adding back Daniel)

Den 10.02.2020 17.57, skrev Noralf Trønnes:
> 
> 
> Den 10.02.2020 16.06, skrev Daniel Vetter:
>> On Mon, Feb 10, 2020 at 12:37:52PM +0100, Gerd Hoffmann wrote:
>>> Move final cleanups to qxl_drm_release() callback.
>>> Add drm_atomic_helper_shutdown() call to qxl_pci_remove().
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  drivers/gpu/drm/qxl/qxl_drv.c | 26 +++---
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
>>> index 1d601f57a6ba..4fda3f9b29f4 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_drv.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
>>> @@ -34,6 +34,7 @@
>>>  #include 
>>>  
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -132,21 +133,30 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *ent)
>>> return ret;
>>>  }
>>>  
>>> +static void qxl_drm_release(struct drm_device *dev)
>>> +{
>>> +   struct qxl_device *qdev = dev->dev_private;
>>> +
>>> +   /*
>>> +* TODO: qxl_device_fini() call should be in qxl_pci_remove(),
>>> +* reodering qxl_modeset_fini() + qxl_device_fini() calls is
>>> +* non-trivial though.
>>> +*/
>>> +   qxl_modeset_fini(qdev);
>>
>> So the drm_mode_config_cleanup call in here belongs in ->release, but the
>> qxl_destroy_monitors_object feels like should be perfectly fine in the
>> remove hook. You might need to sprinkle a few drm_dev_enter/exit around to
>> protect code paths thought.
>>
>> Aside from this lgtm, for the series
>>
>> Acked-by: Daniel Vetter 
>>
>> And up to you whether you want to fix this or not really.
>>
>> Btw for testing all these patches that add a ->release hook I think it'd
>> be good if the drm core checks that drm_device->dev is set to NULL, and
>> that we do this in the remove hook. Since that's guaranteed to be gone at
>> that point, so anything in ->release that still needs the device is
>> broken. Ofc maybe do that check only for drivers which have a ->release
>> hook, and we might need a few fixups.
>>
> 
> We take a ref on the parent device in drm_dev_init() and release it in
> drm_dev_fini(). I added this because of the DRM_DEV_* macros we have, to
> protect access to the device struct after it was unregistered. Setting
> drm_device->dev to NULL in drm_dev_unregister() instead will provide the
> same protection I think.
> 
> commit 56be6503aab2
> drm/drv: Hold ref on parent device during drm_device lifetime
> 
> Noralf.
> 
>> Cheers, Daniel
>>
>>> +   qxl_device_fini(qdev);
>>> +   dev->dev_private = NULL;
>>> +   kfree(qdev);
>>> +}
>>> +
>>>  static void
>>>  qxl_pci_remove(struct pci_dev *pdev)
>>>  {
>>> struct drm_device *dev = pci_get_drvdata(pdev);
>>> -   struct qxl_device *qdev = dev->dev_private;
>>>  
>>> drm_dev_unregister(dev);
>>> -
>>> -   qxl_modeset_fini(qdev);
>>> -   qxl_device_fini(qdev);
>>> +   drm_atomic_helper_shutdown(dev);
>>> if (is_vga(pdev))
>>> vga_put(pdev, VGA_RSRC_LEGACY_IO);
>>> -
>>> -   dev->dev_private = NULL;
>>> -   kfree(qdev);
>>> drm_dev_put(dev);
>>>  }
>>>  
>>> @@ -279,6 +289,8 @@ static struct drm_driver qxl_driver = {
>>> .major = 0,
>>> .minor = 1,
>>> .patchlevel = 0,
>>> +
>>> +   .release = qxl_drm_release,
>>>  };
>>>  
>>>  static int __init qxl_init(void)
>>> -- 
>>> 2.18.1
>>>
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] drm/qxl: add drm_driver.release callback.

[Noralf Trønnes]

Den 10.02.2020 16.06, skrev Daniel Vetter:
> On Mon, Feb 10, 2020 at 12:37:52PM +0100, Gerd Hoffmann wrote:
>> Move final cleanups to qxl_drm_release() callback.
>> Add drm_atomic_helper_shutdown() call to qxl_pci_remove().
>>
>> Signed-off-by: Gerd Hoffmann 
>> ---
>>  drivers/gpu/drm/qxl/qxl_drv.c | 26 +++---
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
>> index 1d601f57a6ba..4fda3f9b29f4 100644
>> --- a/drivers/gpu/drm/qxl/qxl_drv.c
>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
>> @@ -34,6 +34,7 @@
>>  #include 
>>  
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -132,21 +133,30 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  return ret;
>>  }
>>  
>> +static void qxl_drm_release(struct drm_device *dev)
>> +{
>> +struct qxl_device *qdev = dev->dev_private;
>> +
>> +/*
>> + * TODO: qxl_device_fini() call should be in qxl_pci_remove(),
>> + * reodering qxl_modeset_fini() + qxl_device_fini() calls is
>> + * non-trivial though.
>> + */
>> +qxl_modeset_fini(qdev);
> 
> So the drm_mode_config_cleanup call in here belongs in ->release, but the
> qxl_destroy_monitors_object feels like should be perfectly fine in the
> remove hook. You might need to sprinkle a few drm_dev_enter/exit around to
> protect code paths thought.
> 
> Aside from this lgtm, for the series
> 
> Acked-by: Daniel Vetter 
> 
> And up to you whether you want to fix this or not really.
> 
> Btw for testing all these patches that add a ->release hook I think it'd
> be good if the drm core checks that drm_device->dev is set to NULL, and
> that we do this in the remove hook. Since that's guaranteed to be gone at
> that point, so anything in ->release that still needs the device is
> broken. Ofc maybe do that check only for drivers which have a ->release
> hook, and we might need a few fixups.
> 

We take a ref on the parent device in drm_dev_init() and release it in
drm_dev_fini(). I added this because of the DRM_DEV_* macros we have, to
protect access to the device struct after it was unregistered. Setting
drm_device->dev to NULL in drm_dev_unregister() instead will provide the
same protection I think.

commit 56be6503aab2
drm/drv: Hold ref on parent device during drm_device lifetime

Noralf.

> Cheers, Daniel
> 
>> +qxl_device_fini(qdev);
>> +dev->dev_private = NULL;
>> +kfree(qdev);
>> +}
>> +
>>  static void
>>  qxl_pci_remove(struct pci_dev *pdev)
>>  {
>>  struct drm_device *dev = pci_get_drvdata(pdev);
>> -struct qxl_device *qdev = dev->dev_private;
>>  
>>  drm_dev_unregister(dev);
>> -
>> -qxl_modeset_fini(qdev);
>> -qxl_device_fini(qdev);
>> +drm_atomic_helper_shutdown(dev);
>>  if (is_vga(pdev))
>>  vga_put(pdev, VGA_RSRC_LEGACY_IO);
>> -
>> -dev->dev_private = NULL;
>> -kfree(qdev);
>>  drm_dev_put(dev);
>>  }
>>  
>> @@ -279,6 +289,8 @@ static struct drm_driver qxl_driver = {
>>  .major = 0,
>>  .minor = 1,
>>  .patchlevel = 0,
>> +
>> +.release = qxl_drm_release,
>>  };
>>  
>>  static int __init qxl_init(void)
>> -- 
>> 2.18.1
>>
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

[Noralf Trønnes]

Den 07.02.2020 09.41, skrev Thomas Zimmermann:
> The simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_encoder.c | 116 ++
>  include/drm/drm_encoder.h |  10 +++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c



> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for
> + *default name
> + *
> + * Allocates and initialises an encoder that has no further functionality. 
> The
> + * encoder will be released automatically.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> +   int encoder_type,
> +   const char *name, ...)
> +{
> + char *namestr = NULL;
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = devm_kzalloc(dev->dev, sizeof(*encoder), GFP_KERNEL);

The encoder can outlive the devres if the device is unbound when
userspace has open fds, so you can't use devm_ here.

Noralf.

> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> +
> + if (name) {
> + va_list ap;
> +
> + va_start(ap, name);
> + namestr = kvasprintf(GFP_KERNEL, name, ap);
> + va_end(ap);
> + if (!namestr) {
> + ret = -ENOMEM;
> + goto err_devm_kfree;
> + }
> + }
> +
> + ret = __drm_encoder_init(dev, encoder,
> +  _simple_encoder_funcs_destroy,
> +  encoder_type, namestr);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + if (name)
> + kfree(namestr);
> +err_devm_kfree:
> + devm_kfree(dev->dev, encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config

[Noralf Trønnes]

Den 05.07.2019 11.26, skrev Thomas Zimmermann:
> Generic framebuffer emulation uses a shadow buffer for framebuffers with
> dirty() function. If drivers want to use the shadow FB without such a
> function, they can now set prefer_shadow or prefer_shadow_fbdev in their
> mode_config structures. The former flag is exported to userspace, the latter
> flag is fbdev-only.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 19 ++-
>  include/drm/drm_mode_config.h   |  5 +
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7ba6a0255821..56ef169e1814 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -421,7 +421,9 @@ static void drm_fb_helper_dirty_work(struct work_struct 
> *work)
>   return;
>   drm_fb_helper_dirty_blit_real(helper, _copy);
>   }
> - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
> + if (helper->fb->funcs->dirty)
> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
> +  _copy, 1);
>  
>   if (helper->buffer)
>   drm_client_buffer_vunmap(helper->buffer);
> @@ -620,9 +622,6 @@ static void drm_fb_helper_dirty(struct fb_info *info, u32 
> x, u32 y,
>   struct drm_clip_rect *clip = >dirty_clip;
>   unsigned long flags;
>  
> - if (!helper->fb->funcs->dirty)
> - return;

drm_fb_helper_dirty() is called unconditionally by
drm_fb_helper_sys_imageblit() et al, so we need check with
drm_fbdev_use_shadow_fb() here.

> -
>   spin_lock_irqsave(>dirty_lock, flags);
>   clip->x1 = min_t(u32, clip->x1, x);
>   clip->y1 = min_t(u32, clip->y1, y);
> @@ -2166,6 +2165,16 @@ static struct fb_deferred_io drm_fbdev_defio = {
>   .deferred_io= drm_fb_helper_deferred_io,
>  };
>  
> +static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
> +{
> + struct drm_device *dev = fb_helper->dev;
> + struct drm_framebuffer *fb = fb_helper->fb;
> +
> + return dev->mode_config.prefer_shadow_fbdev |
> +dev->mode_config.prefer_shadow |

Use logical OR here

> +!!fb->funcs->dirty;

and you can drop the the double NOT here.

> +}
> +
>  /**
>   * drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
>   * @fb_helper: fbdev helper structure
> @@ -2213,7 +,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper 
> *fb_helper,
>  
>   drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>  
> - if (fb->funcs->dirty) {
> + if (drm_fbdev_use_shadow_fb(fb_helper)) {
>   struct fb_ops *fbops;
>   void *shadow;
>  
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 759d462d028b..e1c751aca353 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -347,6 +347,8 @@ struct drm_mode_config_funcs {
>   * @output_poll_work: delayed work for polling in process context
>   * @preferred_depth: preferred RBG pixel depth, used by fb helpers
>   * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
> + * @prefer_shadow_fbdev: hint to framebuffer emulation to prefer shadow-fb \
> + rendering

It's preferred to have the doc together with the struct member. This way
it's less likely to be forgotten when things change. And we don't use
line cont. when the doc line is too long. Just continue on the next line
after an asterix.

>   * @cursor_width: hint to userspace for max cursor width
>   * @cursor_height: hint to userspace for max cursor height
>   * @helper_private: mid-layer private data
> @@ -852,6 +854,9 @@ struct drm_mode_config {
>   /* dumb ioctl parameters */
>   uint32_t preferred_depth, prefer_shadow;
>  
> + /* fbdev parameters */

No need for this comment.

Doc can look like this, I've done s/framebuffer/fbdev/:
/**
     * @prefer_shadow_fbdev:
 *
 * Hint to fbdev emulation to prefer shadow-fb rendering.
 */

> + uint32_t prefer_shadow_fbdev;

Use bool here.

With that:

Reviewed-by: Noralf Trønnes 

I have tested this on 2 drivers that use generic fbdev: vc4 (no shadow
buf) and mi0283qt which has a dirty callback.

Tested-by: Noralf Trønnes 

> +
>   /**
>* @quirk_addfb_prefer_xbgr_30bpp:
>*
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] drm/fb-helper: Map DRM client buffer only when required

[Noralf Trønnes]

Den 05.07.2019 11.26, skrev Thomas Zimmermann:
> This patch changes DRM clients to not map the buffer by default. The
> buffer, like any buffer object, should be mapped and unmapped when
> needed.
> 
> An unmapped buffer object can be evicted to system memory and does
> not consume video ram until displayed. This allows to use generic fbdev
> emulation with drivers for low-memory devices, such as ast and mgag200.
> 
> This change affects the generic framebuffer console. HW-based consoles
> map their console buffer once and keep it mapped. Userspace can mmap this
> buffer into its address space. The shadow-buffered framebuffer console
> only needs the buffer object to be mapped during updates. While not being
> updated from the shadow buffer, the buffer object can remain unmapped.
> Userspace will always mmap the shadow buffer.
> 
> v2:
>   * change DRM client to not map buffer by default
>   * manually map client buffer for fbdev with HW framebuffer
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] drm/client: Support unmapping of DRM client buffers

[Noralf Trønnes]

Den 05.07.2019 11.26, skrev Thomas Zimmermann:
> DRM clients, such as the fbdev emulation, have their buffer objects
> mapped by default. Mapping a buffer implicitly prevents its relocation.
> Hence, the buffer may permanently consume video memory while it's
> allocated. This is a problem for drivers of low-memory devices, such as
> ast, mgag200 or older framebuffer hardware, which will then not have
> enough memory to display other content (e.g., X11).
> 
> This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
> DRM clients can use these functions to unmap and remap buffer objects
> as needed.
> 
> There's no reference counting for vmap operations. Callers are expected
> to either keep buffers mapped (as it is now), or call vmap and vunmap
> in pairs around code that accesses the mapped memory.
> 
> v2:
>   * remove several duplicated NULL-pointer checks
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_client.c | 67 ++--
>  include/drm/drm_client.h |  3 ++
>  2 files changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 410572f14257..66d8d645ac79 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -281,6 +281,43 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>  
>   buffer->gem = obj;
>  
> + vaddr = drm_client_buffer_vmap(buffer);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_delete;
> + }
> +
> + return buffer;
> +
> +err_delete:
> + drm_client_buffer_delete(buffer);
> +
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * drm_client_buffer_vmap - Map DRM client buffer into address space
> + * @buffer: DRM client buffer
> + *
> + * This function maps a client buffer into kernel address space. If the
> + * buffer is already mapped, it returns the mapping's address.
> + *
> + * Client buffer mappings are not ref'counted. Each call to
> + * drm_client_buffer_vmap() should be followed by a call to
> + * drm_client_buffer_vunmap(); or the client buffer should be mapped
> + * throughout its lifetime. The latter is the default.
> + *
> + * Returns:
> + *   The mapped memory's address
> + */
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)

I prefer to have this on one line.

> +{
> + void *vaddr;
> +
> + if (buffer->vaddr)
> + return buffer->vaddr;
> +
>   /*
>* FIXME: The dependency on GEM here isn't required, we could
>* convert the driver handle to a dma-buf instead and use the
> @@ -289,21 +326,31 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - vaddr = drm_gem_vmap(obj);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - goto err_delete;
> - }
> + vaddr = drm_gem_vmap(buffer->gem);
> + if (IS_ERR(vaddr))
> + return vaddr;
>  
>   buffer->vaddr = vaddr;
>  
> - return buffer;
> -
> -err_delete:
> - drm_client_buffer_delete(buffer);
> + return vaddr;
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vmap);
>  
> - return ERR_PTR(ret);
> +/**
> + * drm_client_buffer_vunmap - Unmap DRM client buffer
> + * @buffer: DRM client buffer
> + *
> + * This function removes a client buffer's memory mmapping. This

s/mmapping/mapping/

> + * function is only required by clients that manage their buffers
> + * by themselves. By default, DRM client buffers are mapped throughout
> + * their entire lifetime.
> + */
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> +{
> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + buffer->vaddr = NULL;
>  }
> +EXPORT_SYMBOL(drm_client_buffer_vunmap);
>  
>  static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
>  {
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 72d51d1e9dd9..e1db1d9da0bf 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -149,6 +149,9 @@ struct drm_client_buffer {
>  struct drm_client_buffer *
>  drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 
> height, u32 format);
>  void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer);

Prefer to have this on one line.

Reviewed-by: Noralf Trønnes 

> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
>  
>  int drm_client_modeset_create(struct drm_client_dev *client);
>  void drm_client_modeset_free(struct drm_client_dev *client);
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[Noralf Trønnes]

Den 04.07.2019 13.10, skrev Thomas Zimmermann:
> Hi
> 
> Am 04.07.19 um 12:18 schrieb Noralf Trønnes:
>>
>>
>> Den 04.07.2019 09.43, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 03.07.19 um 21:27 schrieb Noralf Trønnes:
>>>>
>>>>
>>>> Den 03.07.2019 10.32, skrev Thomas Zimmermann:
>>>>> DRM client buffers are permanently mapped throughout their lifetime. This
>>>>> prevents us from using generic framebuffer emulation for devices with
>>>>> small dedicated video memory, such as ast or mgag200. With fb buffers
>>>>> permanently mapped, such devices often won't have enougth space left to
>>>>> display other content (e.g., X11).
>>>>>
>>>>> This patch set introduces unmappable DRM client buffers for framebuffer
>>>>> emulation with shadow buffers. While the shadow buffer remains in system
>>>>> memory permanently, the respective buffer object will only be mapped 
>>>>> briefly
>>>>> during updates from the shadow buffer. Hence, the driver can relocate he
>>>>> buffer object among memory regions as needed.
>>>>>
>>>>> The default behoviour for DRM client buffers is still to be permanently
>>>>> mapped.
>>>>>
>>>>> The patch set converts ast and mgag200 to generic framebuffer emulation
>>>>> and removes a large amount of framebuffer code from these drivers. For
>>>>> bochs, a problem was reported where the driver could not display the 
>>>>> console
>>>>> because it was pinned in system memory. [1] The patch set fixes this bug
>>>>> by converting bochs to use the shadow fb.
>>>>>
>>>>> The patch set has been tested on ast and mga200 HW.
>>>>>
>>>>
>>>> I've been thinking, this enables dirty tracking in DRM userspace for
>>>> these drivers since the dirty callback is now set on all framebuffers.
>>>> Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
>>>> flag enabling the shadow buffer instead?
>>>
>>> Fbdev emulation is special wrt framebuffer setup and there's no way to
>>> distinguish a regular FB from the fbdev's FB. I've been trying
>>> drm_fbdev_generic_shadow_setup(), but ended up duplicating
>>> functionality. The problem was that we cannot get state-flag arguments
>>> into drm_fb_helper_generic_probe().
>>>
>>> There already is struct mode_config.prefer_shadow. It signals shadow FB
>>> rendering to userspace. The easiest solution is to add
>>> prefer_shadow_fbdev as well. If either flag is true, fbdev emulation
>>> would enable shadow buffering.
>>
>> How about something like this:
> 
> I had something like that in mind, but maybe without a separate
> function. I'll post my variant as part of the patch set's next iteration.
> 



>>>
>>> I'm not sure if we should check for the dirty() callback at all.
>>>
>>
>> Hm, why do you think that?
> 
> Drivers may already come with their own shadow buffer. Cirrus is an
> example of that. It uses shmem buffer objects as shadow fbs and
> internally updates the device frame buffer in its dirty callback. Using
> dirty() to select the shadow fbdev adds another buffer (and another
> memcpy) for no reason.

Cirruc uses shmem buffers and they won't work with fbdev defio (both use
page->lru). shmem is the reason I added shadow buffering to generic
fbdev in the first place. It will now work with whatever backing buffer
the driver uses, as long as it can provide a virtual address on the dumb
buffer (not the case with rockchip for instance, due to limited virtual
address space).

Noralf.

> 
> Best regards
> Thomas
> 
>> The thing with fbdev defio is that it only supports kmalloc and vmalloc
>> allocated memory (page->lru is avail.). This means that only the CMA
>> drivers can use defio without shadow memory. To keep things simple
>> everyone with a dirty() callback gets a shadow buffer.
>>
>> Noralf.
>>
>>> Best regards
>>> Thomas
>>>
>>>> Really nice diffstat by the way :-)
>>>>
>>>> Noralf.
>>>>
>>>>> [1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html
>>>>>
>>>>> Thomas Zimmermann (5):
>>>>>   drm/client: Support unmapping of DRM client buffers
>>>>>   drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
>>&

Re: [PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[Noralf Trønnes]

Den 04.07.2019 09.43, skrev Thomas Zimmermann:
> Hi
> 
> Am 03.07.19 um 21:27 schrieb Noralf Trønnes:
>>
>>
>> Den 03.07.2019 10.32, skrev Thomas Zimmermann:
>>> DRM client buffers are permanently mapped throughout their lifetime. This
>>> prevents us from using generic framebuffer emulation for devices with
>>> small dedicated video memory, such as ast or mgag200. With fb buffers
>>> permanently mapped, such devices often won't have enougth space left to
>>> display other content (e.g., X11).
>>>
>>> This patch set introduces unmappable DRM client buffers for framebuffer
>>> emulation with shadow buffers. While the shadow buffer remains in system
>>> memory permanently, the respective buffer object will only be mapped briefly
>>> during updates from the shadow buffer. Hence, the driver can relocate he
>>> buffer object among memory regions as needed.
>>>
>>> The default behoviour for DRM client buffers is still to be permanently
>>> mapped.
>>>
>>> The patch set converts ast and mgag200 to generic framebuffer emulation
>>> and removes a large amount of framebuffer code from these drivers. For
>>> bochs, a problem was reported where the driver could not display the console
>>> because it was pinned in system memory. [1] The patch set fixes this bug
>>> by converting bochs to use the shadow fb.
>>>
>>> The patch set has been tested on ast and mga200 HW.
>>>
>>
>> I've been thinking, this enables dirty tracking in DRM userspace for
>> these drivers since the dirty callback is now set on all framebuffers.
>> Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
>> flag enabling the shadow buffer instead?
> 
> Fbdev emulation is special wrt framebuffer setup and there's no way to
> distinguish a regular FB from the fbdev's FB. I've been trying
> drm_fbdev_generic_shadow_setup(), but ended up duplicating
> functionality. The problem was that we cannot get state-flag arguments
> into drm_fb_helper_generic_probe().
> 
> There already is struct mode_config.prefer_shadow. It signals shadow FB
> rendering to userspace. The easiest solution is to add
> prefer_shadow_fbdev as well. If either flag is true, fbdev emulation
> would enable shadow buffering.

How about something like this:

diff --git a/drivers/gpu/drm/drm_fb_helper.c
b/drivers/gpu/drm/drm_fb_helper.c
index 1984e5c54d58..723fe56aa5f5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -415,7 +415,8 @@ static void drm_fb_helper_dirty_work(struct
work_struct *work)
/* Generic fbdev uses a shadow buffer */
if (helper->buffer)
drm_fb_helper_dirty_blit_real(helper, _copy);
-   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
+   if (helper->fb->funcs->dirty)
+   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, 
_copy, 1);
}
 }

@@ -2209,7 +2210,7 @@ int drm_fb_helper_generic_probe(struct
drm_fb_helper *fb_helper,
 #endif
drm_fb_helper_fill_info(fbi, fb_helper, sizes);

-   if (fb->funcs->dirty) {
+   if (fb->funcs->dirty || fb_helper->use_shadow) {
struct fb_ops *fbops;
void *shadow;

@@ -2310,6 +2311,44 @@ static const struct drm_client_funcs
drm_fbdev_client_funcs = {
.hotplug= drm_fbdev_client_hotplug,
 };

+static int _drm_fbdev_generic_setup(struct drm_device *dev, unsigned
int preferred_bpp, bool use_shadow)
+{
+   struct drm_fb_helper *fb_helper;
+   int ret;
+
+   WARN(dev->fb_helper, "fb_helper is already set!\n");
+
+   if (!drm_fbdev_emulation)
+   return 0;
+
+   fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
+   if (!fb_helper)
+   return -ENOMEM;
+
+   ret = drm_client_init(dev, _helper->client, "fbdev",
_fbdev_client_funcs);
+   if (ret) {
+   kfree(fb_helper);
+   DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
+   return ret;
+   }
+
+   if (!preferred_bpp)
+   preferred_bpp = dev->mode_config.preferred_depth;
+   if (!preferred_bpp)
+   preferred_bpp = 32;
+   fb_helper->preferred_bpp = preferred_bpp;
+
+   fb_helper->use_shadow = use_shadow;
+
+   ret = drm_fbdev_client_hotplug(_helper->client);
+   if (ret)
+   DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
+
+   drm_client_register(_helper->client);
+
+   return 0;
+}
+
 /**
  * drm_fbdev_generic_setup() - Setup generic fbdev emulation
  * @dev: D

Re: [PATCH 0/5] Unmappable DRM client buffers for fbdev emulation

[Noralf Trønnes]

Den 03.07.2019 10.32, skrev Thomas Zimmermann:
> DRM client buffers are permanently mapped throughout their lifetime. This
> prevents us from using generic framebuffer emulation for devices with
> small dedicated video memory, such as ast or mgag200. With fb buffers
> permanently mapped, such devices often won't have enougth space left to
> display other content (e.g., X11).
> 
> This patch set introduces unmappable DRM client buffers for framebuffer
> emulation with shadow buffers. While the shadow buffer remains in system
> memory permanently, the respective buffer object will only be mapped briefly
> during updates from the shadow buffer. Hence, the driver can relocate he
> buffer object among memory regions as needed.
> 
> The default behoviour for DRM client buffers is still to be permanently
> mapped.
> 
> The patch set converts ast and mgag200 to generic framebuffer emulation
> and removes a large amount of framebuffer code from these drivers. For
> bochs, a problem was reported where the driver could not display the console
> because it was pinned in system memory. [1] The patch set fixes this bug
> by converting bochs to use the shadow fb.
> 
> The patch set has been tested on ast and mga200 HW.
> 

I've been thinking, this enables dirty tracking in DRM userspace for
these drivers since the dirty callback is now set on all framebuffers.
Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
flag enabling the shadow buffer instead?

Really nice diffstat by the way :-)

Noralf.

> [1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html
> 
> Thomas Zimmermann (5):
>   drm/client: Support unmapping of DRM client buffers
>   drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
>   drm/ast: Replace struct ast_fbdev with generic framebuffer emulation
>   drm/bochs: Use shadow buffer for bochs framebuffer console
>   drm/mgag200: Replace struct mga_fbdev with generic framebuffer
> emulation
> 
>  drivers/gpu/drm/ast/Makefile   |   2 +-
>  drivers/gpu/drm/ast/ast_drv.c  |  22 +-
>  drivers/gpu/drm/ast/ast_drv.h  |  17 --
>  drivers/gpu/drm/ast/ast_fb.c   | 341 -
>  drivers/gpu/drm/ast/ast_main.c |  30 ++-
>  drivers/gpu/drm/ast/ast_mode.c |  21 --
>  drivers/gpu/drm/bochs/bochs_kms.c  |   2 +-
>  drivers/gpu/drm/drm_client.c   |  71 -
>  drivers/gpu/drm/drm_fb_helper.c|  14 +-
>  drivers/gpu/drm/mgag200/Makefile   |   2 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 --
>  drivers/gpu/drm/mgag200/mgag200_fb.c   | 309 --
>  drivers/gpu/drm/mgag200/mgag200_main.c |  61 +++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c |  27 --
>  include/drm/drm_client.h   |   3 +
>  15 files changed, 154 insertions(+), 787 deletions(-)
>  delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c
> 
> --
> 2.21.0
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 5/5] drm/mgag200: Replace struct mga_fbdev with generic framebuffer emulation

[Noralf Trønnes]

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> This patch replaces mgag200's framebuffer console with DRM's generic
> implememtation. All respective code is being removed from the driver.
> 
> The console is set up with a shadow buffer. The actual buffer object is
> not permanently pinned in video ram, but just another buffer object that
> the driver moves in and out of vram as necessary. The driver's function
> mga_crtc_do_set_base() used to contain special handling for the framebuffer
> console. With the new generic framebuffer, the driver does not need this
> code an longer.
> 
> Signed-off-by: Thomas Zimmermann 
> ---



> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c 
> b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b10f7265b5c4..6d943a008752 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -14,8 +14,33 @@
>  
>  #include "mgag200_drv.h"
>  
> +static int mga_framebuffer_dirtyfb(struct drm_framebuffer *fb,
> +struct drm_file *file_priv,
> +unsigned int flags,
> +unsigned int color,
> +struct drm_clip_rect *clips,
> +unsigned int num_clips)
> +{
> + /* empty placeholder function to enable fbcon shadow buffer */
> + return 0;
> +}
> +
> +static const struct drm_framebuffer_funcs mga_framebuffer_funcs = {
> + .destroy= drm_gem_fb_destroy,
> + .create_handle  = drm_gem_fb_create_handle,
> + .dirty  = mga_framebuffer_dirtyfb,
> +};
> +
> +static struct drm_framebuffer *
> +mga_mode_config_funcs_fb_create(struct drm_device *dev, struct drm_file 
> *file,
> + const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> + _framebuffer_funcs);
> +}
> +
>  static const struct drm_mode_config_funcs mga_mode_funcs = {
> - .fb_create = drm_gem_fb_create
> + .fb_create = mga_mode_config_funcs_fb_create
>  };
>  
>  static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
> @@ -162,7 +187,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned 
> long flags)
>   if (IS_G200_SE(mdev) && mdev->mc.vram_size < (2048*1024))
>   dev->mode_config.preferred_depth = 16;
>   else
> - dev->mode_config.preferred_depth = 24;
> + dev->mode_config.preferred_depth = 32;

Please mention this change and the reason for it in the commit message.

>   dev->mode_config.prefer_shadow = 1;
>  
>   r = mgag200_modeset_init(mdev);
> @@ -186,6 +211,13 @@ int mgag200_driver_load(struct drm_device *dev, unsigned 
> long flags)
>   }
>   mdev->cursor.pixels_current = NULL;
>  
> + r = drm_fbdev_generic_setup(mdev->dev, 0);
> + if (r) {
> + dev_err(>pdev->dev,
> + "drm_fbdev_generic_setup failed: %d\n", r);

drm_fbdev_generic_setup() will print an error message on failure so you
don't need to do it.

Acked-by: Noralf Trønnes 

> + goto err_modeset;
> + }
> +
>   return 0;
>  
>  err_modeset:
> @@ -204,32 +236,7 @@ void mgag200_driver_unload(struct drm_device *dev)
>   if (mdev == NULL)
>   return;
>   mgag200_modeset_fini(mdev);
> - mgag200_fbdev_fini(mdev);
>   drm_mode_config_cleanup(dev);
>   mgag200_mm_fini(mdev);
>   dev->dev_private = NULL;
>  }
> -
> -int mgag200_gem_create(struct drm_device *dev,
> -u32 size, bool iskernel,
> -struct drm_gem_object **obj)
> -{
> - struct drm_gem_vram_object *gbo;
> - int ret;
> -
> - *obj = NULL;
> -
> - size = roundup(size, PAGE_SIZE);
> - if (size == 0)
> - return -EINVAL;
> -
> - gbo = drm_gem_vram_create(dev, >vram_mm->bdev, size, 0, false);
> - if (IS_ERR(gbo)) {
> - ret = PTR_ERR(gbo);
> - if (ret != -ERESTARTSYS)
> - DRM_ERROR("failed to allocate GEM object\n");
> - return ret;
> - }
> - *obj = >gem;
> - return 0;
> -}
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/5] drm/bochs: Use shadow buffer for bochs framebuffer console

[Noralf Trønnes]

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> The bochs driver (and virtual hardware) requires buffer objects to
> reside in video ram to display them to the screen. So it can not
> display the framebuffer console because the respective buffer object
> is permanently pinned in system memory.
> 
> Using a shadow buffer for the console solves this problem. The console
> emulation will pin the buffer object only during updates from the shadow
> buffer. Otherwise, the bochs driver can freely relocated the buffer
> between system memory and video ram.
> 
> Signed-off-by: Thomas Zimmermann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/5] drm/ast: Replace struct ast_fbdev with generic framebuffer emulation

[Noralf Trønnes]

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> This patch replaces ast's framebuffer console with DRM's generic
> implememtation. All respective code is being removed from the driver.
> 
> The console is set up with a shadow buffer. The actual buffer object is
> not permanently pinned in video ram, but just another buffer object that
> the driver moves in and out of vram as necessary. The driver's function
> ast_crtc_do_set_base() used to contain special handling for the framebuffer
> console. With the new generic framebuffer, the driver does not need this
> code an longer.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/ast/Makefile   |   2 +-
>  drivers/gpu/drm/ast/ast_drv.c  |  22 ++-
>  drivers/gpu/drm/ast/ast_drv.h  |  17 --
>  drivers/gpu/drm/ast/ast_fb.c   | 341 -
>  drivers/gpu/drm/ast/ast_main.c |  30 ++-
>  drivers/gpu/drm/ast/ast_mode.c |  21 --
>  6 files changed, 41 insertions(+), 392 deletions(-)
>  delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
> 
> diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
> index b086dae17013..561f7c4199e4 100644
> --- a/drivers/gpu/drm/ast/Makefile
> +++ b/drivers/gpu/drm/ast/Makefile
> @@ -3,6 +3,6 @@
>  # Makefile for the drm device driver.  This driver provides support for the
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
> -ast-y := ast_drv.o ast_main.o ast_mode.o ast_fb.o ast_ttm.o ast_post.o 
> ast_dp501.o
> +ast-y := ast_drv.o ast_main.o ast_mode.o ast_ttm.o ast_post.o ast_dp501.o
>  
>  obj-$(CONFIG_DRM_AST) := ast.o
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 3811997e78c4..75f55ac1a218 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -103,25 +103,29 @@ static int ast_drm_freeze(struct drm_device *dev)
>  
>   pci_save_state(dev->pdev);
>  
> - console_lock();
> - ast_fbdev_set_suspend(dev, 1);
> - console_unlock();
> + if (dev->fb_helper) {
> + console_lock();
> + drm_fb_helper_set_suspend(dev->fb_helper, 1);
> + console_unlock();
> + }

You can call drm_fb_helper_set_suspend_unlocked() unconditionally here
and it will do the NULL check and locking for you.

> +
>   return 0;
>  }
>  
>  static int ast_drm_thaw(struct drm_device *dev)
>  {
> - int error = 0;
> -
>   ast_post_gpu(dev);
>  
>   drm_mode_config_reset(dev);
>   drm_helper_resume_force_mode(dev);
>  
> - console_lock();
> - ast_fbdev_set_suspend(dev, 0);
> - console_unlock();
> - return error;
> + if (dev->fb_helper) {
> + console_lock();
> + drm_fb_helper_set_suspend(dev->fb_helper, 0);
> + console_unlock();
> + }

Same here.

With that:

Acked-by: Noralf Trønnes 

> +
> + return 0;
>  }
>  
>  static int ast_drm_resume(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index ca794a3fcf8f..907d7480b7ba 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -81,8 +81,6 @@ enum ast_tx_chip {
>  #define AST_DRAM_4Gx16   7
>  #define AST_DRAM_8Gx16   8
>  
> -struct ast_fbdev;
> -
>  struct ast_private {
>   struct drm_device *dev;
>  
> @@ -96,8 +94,6 @@ struct ast_private {
>   uint32_t mclk;
>   uint32_t vram_size;
>  
> - struct ast_fbdev *fbdev;
> -
>   int fb_mtrr;
>  
>   struct drm_gem_object *cursor_cache;
> @@ -239,14 +235,6 @@ struct ast_encoder {
>   struct drm_encoder base;
>  };
>  
> -struct ast_fbdev {
> - struct drm_fb_helper helper; /* must be first */
> - void *sysram;
> - int size;
> - int x1, y1, x2, y2; /* dirty rect */
> - spinlock_t dirty_lock;
> -};
> -
>  #define to_ast_crtc(x) container_of(x, struct ast_crtc, base)
>  #define to_ast_connector(x) container_of(x, struct ast_connector, base)
>  #define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
> @@ -289,11 +277,6 @@ struct ast_vbios_mode_info {
>  extern int ast_mode_init(struct drm_device *dev);
>  extern void ast_mode_fini(struct drm_device *dev);
>  
> -int ast_fbdev_init(struct drm_device *dev);
> -void ast_fbdev_fini(struct drm_device *dev);
> -void ast_fbdev_set_suspend(struct drm_device *dev, int state);
> -void ast_fbdev_set_base(struct ast_private *ast, unsigned long gpu_addr);
> -
>  #define AST_MM_ALIGN_SHIFT 4
>  #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)
>  
> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/

Re: [PATCH 1/5] drm/client: Support unmapping of DRM client buffers

[Noralf Trønnes]

Den 03.07.2019 10.32, skrev Thomas Zimmermann:
> DRM clients, such as the fbdev emulation, have their buffer objects
> mapped by default. Mapping a buffer implicitly prevents its relocation.
> Hence, the buffer may permanently consume video memory while it's
> allocated. This is a problem for drivers of low-memory devices, such as
> ast, mgag200 or older framebuffer hardware, which will then not have
> enough memory to display other content (e.g., X11).
> 
> This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
> DRM clients can use these functions to unmap and remap buffer objects
> as needed.
> 
> There's no reference counting for vmap operations. Callers are expected
> to either keep buffers mapped (as it is now), or call vmap and vunmap
> in pairs around code that accesses the mapped memory.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_client.c | 71 +++-
>  include/drm/drm_client.h |  3 ++
>  2 files changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 410572f14257..d04660c4470a 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -235,7 +235,8 @@ static void drm_client_buffer_delete(struct 
> drm_client_buffer *buffer)
>  {
>   struct drm_device *dev = buffer->client->dev;
>  
> - drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + if (buffer->vaddr)

No need for this, drm_gem_vunmap() has a NULL check.

> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
>  
>   if (buffer->gem)
>   drm_gem_object_put_unlocked(buffer->gem);
> @@ -281,6 +282,43 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>  
>   buffer->gem = obj;
>  
> + vaddr = drm_client_buffer_vmap(buffer);

I think we should change this and _not_ vmap on buffer creation.
Eventually we'll get bootsplash and console clients and they will also
have to deal with these low memory devices. AFAICS the only client that
will need a virtual address at all times is the fbdev client when it
doesn't use a shadow buffer.

> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_delete;
> + }
> +
> + return buffer;
> +
> +err_delete:
> + drm_client_buffer_delete(buffer);
> +
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * drm_client_buffer_vmap - Map DRM client buffer into address space
> + * @buffer: DRM client buffer
> + *
> + * This function maps a client buffer into kernel address space. If the
> + * buffer is already mapped, it returns the mapping's address.
> + *
> + * Client buffer mappings are not ref'counted. Each call to
> + * drm_client_buffer_vmap() should be followed by a call to
> + * drm_client_buffer_vunmap(); or the client buffer should be mapped
> + * throughout its lifetime. The latter is the default.
> + *
> + * Returns:
> + *   The mapped memory's address
> + */
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)
> +{
> + void *vaddr;
> +
> + if (buffer->vaddr)
> + return buffer->vaddr;
> +
>   /*
>* FIXME: The dependency on GEM here isn't required, we could
>* convert the driver handle to a dma-buf instead and use the
> @@ -289,21 +327,34 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - vaddr = drm_gem_vmap(obj);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - goto err_delete;
> - }
> + vaddr = drm_gem_vmap(buffer->gem);
> + if (IS_ERR(vaddr))
> + return vaddr;
>  
>   buffer->vaddr = vaddr;
>  
> - return buffer;
> + return vaddr;
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vmap);
>  
> -err_delete:
> - drm_client_buffer_delete(buffer);
> +/**
> + * drm_client_buffer_vunmap - Unmap DRM client buffer
> + * @buffer: DRM client buffer
> + *
> + * This function removes a client buffer's memory mmapping. This
> + * function is only required by clients that manage their buffers
> + * by themselves. By default, DRM client buffers are mapped throughout
> + * their entire lifetime.
> + */
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> +{
> + if (!buffer->vaddr)

No need for a NULL check here either.

Noralf.

> + return;
>  
> - return ERR_PTR(ret);
> + drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + buffer->vaddr = NULL;
>  }
> +EXPORT_SYMBOL(drm_client_buffer_vunmap);
>  
>  static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
>  {
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 72d51d1e9dd9..e1db1d9da0bf 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -149,6 +149,9 @@ struct drm_client_buffer {
>  

Re: [PATCH] drm/bochs: use simple display pipe

[Noralf Trønnes]

Den 10.04.2019 09.48, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/3] drm: switch drm_fb_xrgb8888_to_rgb565_dstclip to accept __iomem dst

[Noralf Trønnes]

Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way.  The drm format helpers have to use memcpy_toio()
> instead.
> 
> This patch makes drm_fb_xrgb_to_rgb565_dstclip() accept a __iomem
> dst pointer and use memcpy_toio() instead of memcpy().  The helper
> function (drm_fb_xrgb_to_rgb565_line) has been changed to process
> a single scanline.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/drm/drm_format_helper.h |   2 +-
>  drivers/gpu/drm/cirrus/cirrus.c |   2 +-
>  drivers/gpu/drm/drm_format_helper.c | 113 ++--
>  3 files changed, 60 insertions(+), 57 deletions(-)
> 
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index bc2e1004e166..d1b8a9ea01b4 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -23,7 +23,7 @@ void drm_fb_swab16(u16 *dst, void *vaddr, struct 
> drm_framebuffer *fb,
>  void drm_fb_xrgb_to_rgb565(void *dst, void *vaddr,
>  struct drm_framebuffer *fb,
>  struct drm_rect *clip, bool swap);
> -void drm_fb_xrgb_to_rgb565_dstclip(void *dst, unsigned int dst_pitch,
> +void drm_fb_xrgb_to_rgb565_dstclip(void __iomem *dst, unsigned int 
> dst_pitch,
>  void *vaddr, struct drm_framebuffer *fb,
>  struct drm_rect *clip, bool swap);
>  void drm_fb_xrgb_to_rgb888_dstclip(void *dst, unsigned int dst_pitch,
> diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
> index 0fc3aa31b5a4..ed2f2d8cfb6f 100644
> --- a/drivers/gpu/drm/cirrus/cirrus.c
> +++ b/drivers/gpu/drm/cirrus/cirrus.c
> @@ -311,7 +311,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
> vmap, fb, rect);
>  
>   else if (fb->format->cpp[0] == 4 && cirrus->cpp == 2)
> - drm_fb_xrgb_to_rgb565_dstclip(__io_virt(cirrus->vram),
> + drm_fb_xrgb_to_rgb565_dstclip(cirrus->vram,
> cirrus->pitch,
> vmap, fb, rect, false);
>  
> diff --git a/drivers/gpu/drm/drm_format_helper.c 
> b/drivers/gpu/drm/drm_format_helper.c
> index dace05638bc3..c9521af4e90b 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -113,42 +113,22 @@ void drm_fb_swab16(u16 *dst, void *vaddr, struct 
> drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_fb_swab16);
>  
> -static void drm_fb_xrgb_to_rgb565_lines(void *dst, unsigned int 
> dst_pitch,
> - void *src, unsigned int src_pitch,
> - unsigned int src_linelength,
> - unsigned int lines,
> - bool swap)
> +static void drm_fb_xrgb_to_rgb565_line(u16 *dbuf, u32 *sbuf,
> +unsigned int pixels,
> +        bool swab)

Both here and further down you change the argument name: swap -> swab.
If you want that, you need to fix the function declaration and the docs
as well.

With that sorted out:
Reviewed-by: Noralf Trønnes 


>  {
> - unsigned int linepixels = src_linelength / sizeof(u32);
> - unsigned int x, y;
> - u32 *sbuf;
> - u16 *dbuf, val16;
> + unsigned int x;
> + u16 val16;
>  
> - /*
> -  * The cma memory is write-combined so reads are uncached.
> -  * Speed up by fetching one line at a time.
> -  */
> - sbuf = kmalloc(src_linelength, GFP_KERNEL);
> - if (!sbuf)
> - return;
> -
> - for (y = 0; y < lines; y++) {
> - memcpy(sbuf, src, src_linelength);
> - dbuf = dst;
> - for (x = 0; x < linepixels; x++) {
> - val16 = ((sbuf[x] & 0x00F8) >> 8) |
> - ((sbuf[x] & 0xFC00) >> 5) |
> - ((sbuf[x] & 0x00F8) >> 3);
> - if (swap)
> - *dbuf++ = swab16(val16);
> - else
> - *dbuf++ = val16;
> - }
> - src += src_pitch;
> - dst += dst_pitch;
> + for (x = 0; x < pixels; x++) {
> + val16 = ((sbuf[x] & 0x00F8) >> 8) |
> + ((sbuf[x] & 0xFC00) >> 5) |
> + ((sbuf[x]

Re: [PATCH v2 3/3] drm: switch drm_fb_xrgb8888_to_rgb888_dstclip to accept __iomem dst

[Noralf Trønnes]

Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way.  The drm format helpers have to use memcpy_toio()
> instead.
> 
> This patch makes drm_fb_xrgb_to_rgb888_dstclip() accept a __iomem
> dst pointer and use memcpy_toio() instead of memcpy().  The helper
> function (drm_fb_xrgb_to_rgb888_line) has been changed to process a
> single scanline.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/3] drm: switch drm_fb_memcpy_dstclip to accept __iomem dst

[Noralf Trønnes]

Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way.  The drm format helpers have to use memcpy_toio()
> instead.
> 
> This patch makes drm_fb_memcpy_dstclip() accept a __iomem dst pointer
> and use memcpy_toio() instead of memcpy().  With that separating out the
> memcpy loop into the drm_fb_memcpy_lines() helper isn't useful any more,
> so move the code back into the calling functins.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Reviewed-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 5/5] drm/cirrus: rewrite and modernize driver.

[Noralf Trønnes]

Den 05.04.2019 11.52, skrev Gerd Hoffmann:
> Time to kill some bad sample code people are copying from ;)
> 
> This is a complete rewrite of the cirrus driver.  The cirrus_mode_set()
> function is pretty much the only function which is carried over largely
> unmodified.  Everything else is upside down.
> 
> It is a single monster patch.  But given that it does some pretty
> fundamental changes to the drivers workflow and also reduces the code
> size by roughly 70% I think it'll still be alot easier to review than a
> longish baby-step patch series.
> 
> Changes summary:
>  - Given the small amout of video memory (4 MB) the cirrus device has
>the rewritten driver doesn't try to manage buffers there.  Instead
>it will blit (memcpy) the active framebuffer to video memory.
>  - All gem objects are stored in main memory and are manged using the
>new shmem helpers.  ttm is out.
>  - It supports RG16, RG24 and XR24 formats.  XR24 gets converted to RG24
>or RG16 at blit time if needed, to avoid the pitch becoming larger
>than what the cirrus hardware can handle.
>  - The simple display pipeline is used.
>  - The generic fbdev emulation is used.
>  - It's a atomic driver now.
>  - It runs wayland.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm/cirrus: rewrite and modernize driver.

[Noralf Trønnes]

Den 04.04.2019 12.27, skrev Gerd Hoffmann:
>   Hi,
> 
>>> tinydrm_xrgb_to_*
>>>
>>> imo these could be put into some drm_format_helpers.c to be shared.
>>
>> I agree, my long term goal is to get rid of tinydrm.ko. Just haven't got
>> there yet.
>>
>> Gerd, if you end up using some of those functions, feel free to move
>> just those you need and I can do the rest later. But if you have time to
>> spare I wouldn't mind getting all of them moved ;-)
> 
> For now I just promoted cirrus to be a tinydrm driver ;)
> 
> Noticed that those helpers (including tinydrm_memcpy for the
> non-converting case) apply clipping on the source but not on
> the destination.  So, for fullscreen updates that works ok,
> but for updating sub-rectangles it doesn't ...

Ah yes, these MIPI type controllers support setting the destination
window in controller RAM for the incoming buffer so there has been no
need for clipping on the destination buffer.

> 
> So I guess I have to add a dest_clip bool parameter when moving them.
> /me looks for a good place.  drm_fb_helpers.c I think.
> 
> cheers,
>   Gerd
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm/cirrus: rewrite and modernize driver.

[Noralf Trønnes]

Den 04.04.2019 10.52, skrev Daniel Vetter:
> On Thu, Apr 4, 2019 at 10:30 AM Gerd Hoffmann  wrote:
>>
>>   Hi,
>>
 Speaking of wayland:  Seems at least gnome-shell insists on using XR24.
>>>
>>> Yeah XR24 is pretty much mandatory. Noralf added a few helpers to
>>> convert XR24 to other formats, for display not supporting anything
>>> else. Because userspace.
>>
>> Have a pointer to these helpers?  grepping around in drm didn't turn up
>> anything so far ...
> 
> tinydrm_xrgb_to_*
> 
> imo these could be put into some drm_format_helpers.c to be shared.

I agree, my long term goal is to get rid of tinydrm.ko. Just haven't got
there yet.

Gerd, if you end up using some of those functions, feel free to move
just those you need and I can do the rest later. But if you have time to
spare I wouldn't mind getting all of them moved ;-)

Noralf.

> From a quick look the xrgb_to_rgb888 is missing, but for a quick
> hack you can just use rgb565 to get going.
> -Daniel
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm/cirrus: rewrite and modernize driver.

[Noralf Trønnes]

Den 03.04.2019 10.53, skrev Gerd Hoffmann:
>>> +struct cirrus_device {
>>> +   struct drm_device  *dev;
>>
>> Why not embed drm_device? It's the latest rage :-)
> 
> Sure, can do that.
> 
>>> +void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
>>> +   struct drm_plane_state *old_state)
>>> +{
>>> +   struct drm_plane_state *state = pipe->plane.state;
>>> +   struct drm_crtc *crtc = >crtc;
>>> +   struct drm_rect rect;
>>> +
>>> +   if (drm_atomic_helper_damage_merged(old_state, state, ))
>>> +   cirrus_fb_blit_rect(pipe->plane.state->fb, );
>>> +
>>> +   if (crtc->state->event) {
>>> +   spin_lock_irq(>dev->event_lock);
>>> +   drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>> +   spin_unlock_irq(>dev->event_lock);
>>> +   crtc->state->event = NULL;
>>> +   }
>>> +}
> 
>>> +static int cirrus_fb_dirty(struct drm_framebuffer *fb,
>>> +  struct drm_file *file_priv,
>>> +  unsigned int flags, unsigned int color,
>>> +  struct drm_clip_rect *clips,
>>> +  unsigned int num_clips)
>>> +{
>>> +   struct cirrus_device *cirrus = fb->dev->dev_private;
>>> +
>>> +   if (cirrus->pipe.plane.state->fb != fb)
>>> +   return 0;
>>> +
>>> +   if (num_clips)
>>> +   cirrus_fb_blit_clips(fb, clips, num_clips);
>>> +   else
>>> +   cirrus_fb_blit_fullscreen(fb);
>>> +   return 0;
>>> +}
>>
>> Why not use the dirty helpers and implement dirty rect support in your
>> main plane update function?
> 
> Dirty rect support is already there, see above.
> 
> So I can just remove cirrus_fb_dirty() and hook up
> drm_atomic_helper_dirtyfb() instead.  Easy ;)
> 

You can even use drm_gem_fb_create_with_dirty() instead of
drm_gem_fb_create_with_funcs().

Noralf.

> cheers,
>   Gerd
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 5/5] drm/virtio: rework resource creation workflow.

[Noralf Trønnes]

Den 18.03.2019 12.33, skrev Gerd Hoffmann:
> This patch moves the virtio_gpu_cmd_create_resource() call (which
> notifies the host about the new resource created) into the
> virtio_gpu_object_create() function.  That way we can call
> virtio_gpu_cmd_create_resource() before ttm_bo_init(), so the host
> already knows about the object when ttm initializes the object and calls
> our driver callbacks.
> 
> Specifically the object is already created when the
> virtio_gpu_ttm_tt_bind() callback invokes virtio_gpu_object_attach(),
> so the extra virtio_gpu_object_attach() calls done after
> virtio_gpu_object_create() are not needed any more.
> 
> The fence support for the create ioctl becomes a bit more tricky though.
> The code moved into virtio_gpu_object_create() too.  We first submit the
> (fenced) virtio_gpu_cmd_create_resource() command, then initialize the
> ttm object, and finally attach just created object to the fence for the
> command in case it didn't finish yet.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/5] drm/virtio: use struct to pass params to virtio_gpu_object_create()

[Noralf Trønnes]

Den 18.03.2019 12.33, skrev Gerd Hoffmann:
> Create virtio_gpu_object_params, use that to pass object parameters to
> virtio_gpu_object_create.  This is just the first step, followup patches
> will add more parameters to the struct.  The plan is to use the struct
> for all object parameters.
> 
> Drop unused "kernel" parameter for virtio_gpu_alloc_object(), it is
> unused and always false.
> 
> Also drop "pinned" parameter.  virtio-gpu doesn't shuffle around
> objects, so effecively they all are pinned anyway.  Hardcode
> TTM_PL_FLAG_NO_EVICT so ttm knows.  Doesn't change much for the moment
> as virtio-gpu supports TTM_PL_FLAG_TT only so there is no opportunity to
> move around objects.  That'll probably change in the future though.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm/virtio: add virtio-gpu-features debugfs file.

[Noralf Trønnes]

Den 20.03.2019 09.36, skrev Gerd Hoffmann:
> This file prints which features the virtio-gpu device has.
> 
> Also add "virtio-gpu-" prefix to the existing fence file,
> to make clear this is a driver-specific debugfs file.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] drm/virtio: implement prime export

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Just run drm_prime_pages_to_sg() on the ttm pages list to get an
> sg_table for export.  The pages list is created at object initialization
> time, so there should be no need to handle an unpopulated page list.
> Add a sanity check nevertheless.
> 
> Signed-off-by: Gerd Hoffmann 

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] drm/virtio: implement prime mmap

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Sync gem vm_node.start with ttm vm_node.start,
> then we can just call drm_gem_prime_mmap().
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] drm/virtio: implement prime pin/unpin

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> virtio-gpu objects never move around, so effectively they are pinned
> all the time.  This makes the the implementation pretty easy ;)
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/virtio/virtgpu_prime.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c 
> b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index b4c9199349e7..0fcae0e46abd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -30,13 +30,13 @@
>  
>  int virtgpu_gem_prime_pin(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> - return -ENODEV;
> + /* nothing: all virtio-gpu objects are pinned all the time */
> + return 0;
>  }
>  
>  void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> + /* nothing */
>  }

You can just remove these dummies the callbacks are optional. See
drm_gem_pin().

With that:

Reviewed-by: Noralf Trønnes 

>  
>  void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource_3d()

[Noralf Trønnes]

Den 01.02.2019 09.01, skrev Gerd Hoffmann:
> On Thu, Jan 31, 2019 at 11:47:38AM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 30.01.2019 10.43, skrev Gerd Hoffmann:
>>> Add 3d resource parameters to virtio_gpu_object_params struct.  With
>>> that in place we can use it for virtio_gpu_cmd_resource_create_3d()
>>> calls.
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>
>> You don't remove the struct virtio_gpu_resource_create_3d definition,
>> but it looks like there's no users left?
> 
> virtio_gpu_cmd_resource_create_3d() still uses it (and has to, it is
> part of the guest <-> host protocol).
> 

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource()

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Add format, width and height fields to the virtio_gpu_object_params
> struct.  With that in place we can use the parameter struct for
> virtio_gpu_cmd_create_resource() calls too.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Drop the dummy ttm backend implementation, add a real one for
> TTM_PL_FLAG_TT objects.  The bin/unbind callbacks will call
> virtio_gpu_object_{attach,detach}, to update the object state
> on the host side, instead of invoking those calls from the
> move_notify() callback.
> 
> With that in place the move and move_notify callbacks are not
> needed any more, so drop them.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource_3d()

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Add 3d resource parameters to virtio_gpu_object_params struct.  With
> that in place we can use it for virtio_gpu_cmd_resource_create_3d()
> calls.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

You don't remove the struct virtio_gpu_resource_create_3d definition,
but it looks like there's no users left?

Noralf.

>  drivers/gpu/drm/virtio/virtgpu_drv.h   | 10 +-
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 25 ++---
>  drivers/gpu/drm/virtio/virtgpu_vq.c| 16 +---
>  3 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index a40215c10e..3265e62725 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -56,6 +56,14 @@ struct virtio_gpu_object_params {
>   uint32_t height;
>   unsigned long size;
>   bool pinned;
> + /* 3d */
> + uint32_t target;
> + uint32_t bind;
> + uint32_t depth;
> + uint32_t array_size;
> + uint32_t last_level;
> + uint32_t nr_samples;
> + uint32_t flags;
>  };
>  
>  struct virtio_gpu_object {
> @@ -310,7 +318,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct 
> virtio_gpu_device *vgdev,
>  void
>  virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_object *bo,
> -   struct virtio_gpu_resource_create_3d *rc_3d);
> +   struct virtio_gpu_object_params *params);
>  void virtio_gpu_ctrl_ack(struct virtqueue *vq);
>  void virtio_gpu_cursor_ack(struct virtqueue *vq);
>  void virtio_gpu_fence_ack(struct virtqueue *vq);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 84c2216fd4..431e5d767e 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -283,7 +283,6 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   struct ttm_validate_buffer mainbuf;
>   struct virtio_gpu_fence *fence = NULL;
>   struct ww_acquire_ctx ticket;
> - struct virtio_gpu_resource_create_3d rc_3d;
>   struct virtio_gpu_object_params params = { 0 };
>  
>   if (vgdev->has_virgl_3d == false) {
> @@ -307,7 +306,15 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   params.width = rc->width;
>   params.height = rc->height;
>   params.size = rc->size;
> -
> + if (vgdev->has_virgl_3d) {
> + params.target = rc->target;
> + params.bind = rc->bind;
> + params.depth = rc->depth;
> + params.array_size = rc->array_size;
> + params.last_level = rc->last_level;
> + params.nr_samples = rc->nr_samples;
> + params.flags = rc->flags;
> + }
>   /* allocate a single page size object */
>   if (params.size == 0)
>   params.size = PAGE_SIZE;
> @@ -333,25 +340,13 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   goto fail_unref;
>   }
>  
> - rc_3d.resource_id = cpu_to_le32(qobj->hw_res_handle);
> - rc_3d.target = cpu_to_le32(rc->target);
> - rc_3d.format = cpu_to_le32(rc->format);
> - rc_3d.bind = cpu_to_le32(rc->bind);
> - rc_3d.width = cpu_to_le32(rc->width);
> - rc_3d.height = cpu_to_le32(rc->height);
> - rc_3d.depth = cpu_to_le32(rc->depth);
> - rc_3d.array_size = cpu_to_le32(rc->array_size);
> - rc_3d.last_level = cpu_to_le32(rc->last_level);
> - rc_3d.nr_samples = cpu_to_le32(rc->nr_samples);
> - rc_3d.flags = cpu_to_le32(rc->flags);
> -
>   fence = virtio_gpu_fence_alloc(vgdev);
>   if (!fence) {
>   ret = -ENOMEM;
>   goto fail_backoff;
>   }
>  
> - virtio_gpu_cmd_resource_create_3d(vgdev, qobj, _3d);
> + virtio_gpu_cmd_resource_create_3d(vgdev, qobj, );
>   ret = virtio_gpu_object_attach(vgdev, qobj, fence);
>   if (ret) {
>   dma_fence_put(>f);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c 
> b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 363b8b8577..ca93ec6ca3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -826,7 +826,7 @@ void virtio_gpu_cmd_context_detach_resource(struct 
> virtio_gpu_device *vgdev,
>  void
>  virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_object *bo,
> -   struct virtio_gpu_resource_create_3d *rc_3d)
> +   struct virtio_gpu_object_params *params)
>  {
>   struct 

Re: [PATCH v2 5/6] drm/virtio: drop fencing in virtio_gpu_resource_create_ioctl

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> There is no need to wait for completion here.
> 
> The host will process commands in submit order, so commands can
> reference the new resource just fine even when queued up before
> completion.
> 
> On the guest side there is no need to wait for completion too.  Which
> btw is different from resource destroy, where we have to make sure the
> host has seen the destroy and thus doesn't use it any more before
> releasing the pages backing the resource.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 6/6] drm/virtio: move virtio_gpu_cmd_create_resource call into virtio_gpu_object_create

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Specifically call virtio_gpu_object_create() before ttm_bo_init(), so
> the object is already created when ttm calls the
> virtio_gpu_ttm_tt_bind() callback (which in turn calls
> virtio_gpu_object_attach()).
> 
> With that in place virtio_gpu_object_attach() will never be called with
> an object which is not yet created, so the extra
> virtio_gpu_object_attach() calls done after
> virtio_gpu_cmd_create_resource() is not needed any more.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] drm/virtio: use struct to pass params to virtio_gpu_object_create()

[Noralf Trønnes]

gt; + obj = virtio_gpu_alloc_object(dev, params);
>   if (IS_ERR(obj))
>   return PTR_ERR(obj);
>  
> @@ -85,6 +85,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>   struct virtio_gpu_device *vgdev = dev->dev_private;
>   struct drm_gem_object *gobj;
>   struct virtio_gpu_object *obj;
> + struct virtio_gpu_object_params params = { 0 };
>   int ret;
>   uint32_t pitch;
>   uint32_t format;
> @@ -96,7 +97,9 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>   args->size = pitch * args->height;
>   args->size = ALIGN(args->size, PAGE_SIZE);
>  
> - ret = virtio_gpu_gem_create(file_priv, dev, args->size, ,
> + params.pinned = false,

You have a comma here, but assigning to false isn't really necessary
since the struct is zeroed. Same goes for the same assignment further down.

With this fixed in some way:

Acked-by: Noralf Trønnes 

> + params.size = args->size;
> + ret = virtio_gpu_gem_create(file_priv, dev, , ,
>   >handle);
>   if (ret)
>   goto fail;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 14ce8188c0..fa7b958ca2 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -279,12 +279,12 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   struct virtio_gpu_object *qobj;
>   struct drm_gem_object *obj;
>   uint32_t handle = 0;
> - uint32_t size;
>   struct list_head validate_list;
>   struct ttm_validate_buffer mainbuf;
>   struct virtio_gpu_fence *fence = NULL;
>   struct ww_acquire_ctx ticket;
>   struct virtio_gpu_resource_create_3d rc_3d;
> + struct virtio_gpu_object_params params = { 0 };
>  
>   if (vgdev->has_virgl_3d == false) {
>   if (rc->depth > 1)
> @@ -302,13 +302,14 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   INIT_LIST_HEAD(_list);
>   memset(, 0, sizeof(struct ttm_validate_buffer));
>  
> - size = rc->size;
> + params.pinned = false,
> + params.size = rc->size;
>  
>   /* allocate a single page size object */
> - if (size == 0)
> - size = PAGE_SIZE;
> + if (params.size == 0)
> + params.size = PAGE_SIZE;
>  
> - qobj = virtio_gpu_alloc_object(dev, size, false, false);
> + qobj = virtio_gpu_alloc_object(dev, );
>   if (IS_ERR(qobj))
>   return PTR_ERR(qobj);
>   obj = >gem_base;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c 
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index f39a183d59..62367e3f80 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -79,21 +79,16 @@ static void virtio_gpu_init_ttm_placement(struct 
> virtio_gpu_object *vgbo,
>  }
>  
>  int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
> -  unsigned long size, bool kernel, bool pinned,
> +  struct virtio_gpu_object_params *params,
>struct virtio_gpu_object **bo_ptr)
>  {
>   struct virtio_gpu_object *bo;
> - enum ttm_bo_type type;
>   size_t acc_size;
>   int ret;
>  
> - if (kernel)
> - type = ttm_bo_type_kernel;
> - else
> - type = ttm_bo_type_device;
>   *bo_ptr = NULL;
>  
> - acc_size = ttm_bo_dma_acc_size(>mman.bdev, size,
> + acc_size = ttm_bo_dma_acc_size(>mman.bdev, params->size,
>  sizeof(struct virtio_gpu_object));
>  
>   bo = kzalloc(sizeof(struct virtio_gpu_object), GFP_KERNEL);
> @@ -104,19 +99,20 @@ int virtio_gpu_object_create(struct virtio_gpu_device 
> *vgdev,
>   kfree(bo);
>   return ret;
>   }
> - size = roundup(size, PAGE_SIZE);
> - ret = drm_gem_object_init(vgdev->ddev, >gem_base, size);
> + params->size = roundup(params->size, PAGE_SIZE);
> + ret = drm_gem_object_init(vgdev->ddev, >gem_base, params->size);
>   if (ret != 0) {
>   virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
>   kfree(bo);
>   return ret;
>   }
>   bo->dumb = false;
> - virtio_gpu_init_ttm_placement(bo, pinned);
> + virtio_gpu_init_ttm_placement(bo, params->pinned);
>  
> - ret = ttm_bo_init(>mman.bdev, >tbo, size, type,
> -   >placement, 0, !kernel, acc_size,
> -   NULL, NULL, _gpu_ttm_bo_destroy);
> + ret = ttm_bo_init(>mman.bdev, >tbo, params->size,
> +   ttm_bo_type_device, >placement, 0,
> +   true, acc_size, NULL, NULL,
> +   _gpu_ttm_bo_destroy);
>   /* ttm_bo_init failure will call the destroy */
>   if (ret != 0)
>   return ret;
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] drm/qxl: use ttm_tt

[Noralf Trønnes]

Den 29.01.2019 09.25, skrev Gerd Hoffmann:
> qxl device will not dma, so we don't need ttm_dma_tt.  Go use ttm_tt
> instead, to avoid wasting resources (swiotlb bounce buffers for
> example).
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 17/23] drm/qxl: use generic fbdev emulation

[Noralf Trønnes]

Den 28.01.2019 09.59, skrev Gerd Hoffmann:
> On Fri, Jan 25, 2019 at 06:25:27PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 18.01.2019 13.20, skrev Gerd Hoffmann:
>>> Switch qxl over to the new generic fbdev emulation.
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  drivers/gpu/drm/qxl/qxl_display.c | 7 ---
>>>  drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
>>>  2 files changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
>>> b/drivers/gpu/drm/qxl/qxl_display.c
>>> index ef832f98ab..9c751f01e3 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>>> @@ -1221,18 +1221,11 @@ int qxl_modeset_init(struct qxl_device *qdev)
>>> qxl_display_read_client_monitors_config(qdev);
>>>  
>>> drm_mode_config_reset(>ddev);
>>> -
>>> -   /* primary surface must be created by this point, to allow
>>> -* issuing command queue commands and having them read by
>>> -* spice server. */
>>> -   qxl_fbdev_init(qdev);
>>> return 0;
>>>  }
>>>  
>>>  void qxl_modeset_fini(struct qxl_device *qdev)
>>>  {
>>> -   qxl_fbdev_fini(qdev);
>>> -
>>> qxl_destroy_monitors_object(qdev);
>>> drm_mode_config_cleanup(>ddev);
>>>  }
>>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
>>> index 13c8a662f9..3fce7d16df 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_drv.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
>>> @@ -93,6 +93,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *ent)
>>> if (ret)
>>> goto modeset_cleanup;
>>>  
>>> +   drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
>>
>> I couldn't find that this was part of old fbdev code, so it would be
>> nice to mention it in the commit message.
> 
> It actually is, but a bit hidden because it doesn't use a helper you can
> easily grep for.  Instead sets fb_info->apertures->ranges[0] in
> qxlfb_create(), which has the same effect.
> 

Indeed,

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 10/23] drm/qxl: move qxl_primary_apply_cursor to correct place

[Noralf Trønnes]

Den 28.01.2019 09.10, skrev Gerd Hoffmann:
>>> The cursor must be set again after creating the primary surface.
>>> Also drop the error message.
> 
>>> if (!bo->is_primary) {
>>> -   if (!same_shadow)
>>> +   if (!same_shadow) {
>>> qxl_io_create_primary(qdev, 0, bo);
>>> +   qxl_primary_apply_cursor(plane);
>>> +   }
>>> bo->is_primary = true;
>>> }
>>>  
>>>
>>
>> I don't see how the commit message matches what you're doing. It gives
>> the impression that it must be applied under yet another condition, but
>> the condition for applying the cursor is changed from bo_old->is_primary
>> to !bo->is_primary.
> 
> The qxl device ties the cursor to the primary surface.  Therefore
> calling qxl_io_destroy_primary() and qxl_io_create_primary() to switch
> the framebuffer causes the cursor information being lost and the driver
> must re-apply it.
> 
> The correct call order to do that is qxl_io_destroy_primary() +
> qxl_io_create_primary() + qxl_primary_apply_cursor().
> 
> The old code did qxl_io_destroy_primary() + qxl_primary_apply_cursor() +
> qxl_io_create_primary().  Due to qxl_primary_apply_cursor request being
> queued in a ringbuffer and qxl_io_create_primary() trapping to the
> hypervisor instantly there is a high chance that qxl_io_create_primary()
> is processed first even with the wrong call order.  But it's racy and
> thus not reliable.
> 
>> It probably makes sense to someone that knows the driver.
> 
> If the above explains things better to you I should probably replace the
> commit message with that.
> 

This is actually my first review of a driver that I'm not familiar with.
I'm not quite sure how much in depth understanding that is required to
put my ack on it. Going further into the patchset I realised that
there's no way that I can verify the logic without being intimate with
the driver. So I have tried to verify things from a kms point of view.

I liked your expanded explanation better.

Noralf.

>> Acked-by: Noralf Trønnes 
> 
> thanks,
>   Gerd
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 09/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for dumb gem objects

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> dumb buffers are used as qxl surfaces, so allocate them as
> QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
> PRIV ttm domain then, so this reduces VRAM memory pressure.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code

[Noralf Trønnes]

Den 25.01.2019 19.10, skrev Sam Ravnborg:
> Hi Noralf.
> 
>>> Lovely diffstat, thanks to the new generic fbdev emulation.
>>>
>>>  drm/qxl/Makefile   |2
>>>  drm/qxl/qxl_draw.c |  232 
>>>  drm/qxl/qxl_drv.h  |   21 ---
>>>  drm/qxl/qxl_fb.c   |  300 
>>> -
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  drivers/gpu/drm/qxl/qxl_drv.h  |  21 ---
>>>  drivers/gpu/drm/qxl/qxl_draw.c | 232 ---
>>>  drivers/gpu/drm/qxl/qxl_fb.c   | 300 
>>> -
>>>  drivers/gpu/drm/qxl/Makefile   |   2 +-
>>>  4 files changed, 1 insertion(+), 554 deletions(-)
>>>  delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c
>>>
>>
>> Nice!
>>
>> Acked-by: Noralf Trønnes 
> It must be a very satisfying experience to see such benefits
> of the many hours of works you have put into the genric
> fbdev emulation code.

Indeed and having more users of the code increases the chance of
detecting any bugs lurking underneath the surface.

> Well done, and indeed a nice patch from Gerd.
> 
>   Sam
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 08/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for shadow bo.

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> The shadow bo is used as qxl surface, so allocate it as
> QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
> PRIV ttm domain then, so this reduces VRAM memory pressure.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 07/23] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> qxl surfaces (used for framebuffers and gem objects) can live in both
> VRAM and PRIV ttm domains.  Update placement setup to include both.
> Put PRIV first in the list so it is preferred, so VRAM will have more
> room for objects which must be allocated there.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 06/23] drm/qxl: use separate offset spaces for the two slots / ttm memory types.

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Without that ttm offsets are not unique, they can refer to objects
> in both VRAM and PRIV memory (aka main and surfaces slot).
> 
> One of those "why things didn't blow up without this" moments.
> Probably offset conflicts are rare enough by pure luck.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 05/23] drm/qxl: drop unused fields from struct qxl_device

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> slot_id_bits and slot_gen_bits can be read directly from qxlrom instead.
> va_slot_mask is never used anywhere.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 02/23] drm/qxl: drop unused qxl_fb_virtual_address

[Noralf Trønnes]

Den 18.01.2019 13.19, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 03/23] drm/qxl: simplify slot management

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Drop pointless indirection, remove the mem_slots array and index
> variables, drop dynamic allocation.  Store memslots in qxl_device
> instead.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Looks sane:

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 04/23] drm/qxl: change the way slot is detected

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> From: Frediano Ziglio 
> 
> Instead of relaying on surface type use the actual placement.
> This allow to have different placement for a single type of
> surface.
> 
> Signed-off-by: Frediano Ziglio 
> 
> [ kraxel: rebased, adapted to upstream changes ]
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 01/23] drm/qxl: drop ttm_mem_reg arg from qxl_hw_surface_alloc()

[Noralf Trønnes]

Den 18.01.2019 13.19, skrev Gerd Hoffmann:
> Not used, is always NULL.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 22/23] drm/qxl: use kernel mode db

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add all standard modes from the kernel's video mode data base.
> Keep a few non-standard modes in the qxl mode list.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 21/23] drm/qxl: add qxl_add_mode helper function

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add a helper function to add custom video modes to a connector.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 20/23] drm/qxl: add mode/framebuffer check functions

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add a helper functions to check video modes.  Also add a helper to check
> framebuffer buffer objects, using the former for consistency.  That way
> we should not fail in qxl_primary_atomic_check() because video modes
> which are too big will not be added to the mode list in the first place.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 19/23] drm/qxl: implement qxl_gem_prime_(un)pin

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Lovely diffstat, thanks to the new generic fbdev emulation.
> 
>  drm/qxl/Makefile   |2
>  drm/qxl/qxl_draw.c |  232 
>  drm/qxl/qxl_drv.h  |   21 ---
>  drm/qxl/qxl_fb.c   |  300 
> -
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_drv.h  |  21 ---
>  drivers/gpu/drm/qxl/qxl_draw.c | 232 ---
>  drivers/gpu/drm/qxl/qxl_fb.c   | 300 
> -
>  drivers/gpu/drm/qxl/Makefile   |   2 +-
>  4 files changed, 1 insertion(+), 554 deletions(-)
>  delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c
> 

Nice!

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 15/23] drm/qxl: use qxl_num_crtc directly

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> qdev->monitors_config->max_allowed is effectively set by the
> qxl.num_heads module parameter, stored in the qxl_num_crtc variable.
> Lets get rid of the indirection and use the variable qxl_num_crtc
> directly.  The kernel doesn't need to dereference pointers each time it
> needs the value, and when reading the code you don't have to trace where
> and why qdev->monitors_config->max_allowed is set.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 17/23] drm/qxl: use generic fbdev emulation

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Switch qxl over to the new generic fbdev emulation.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 7 ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ef832f98ab..9c751f01e3 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1221,18 +1221,11 @@ int qxl_modeset_init(struct qxl_device *qdev)
>   qxl_display_read_client_monitors_config(qdev);
>  
>   drm_mode_config_reset(>ddev);
> -
> - /* primary surface must be created by this point, to allow
> -  * issuing command queue commands and having them read by
> -  * spice server. */
> - qxl_fbdev_init(qdev);
>   return 0;
>  }
>  
>  void qxl_modeset_fini(struct qxl_device *qdev)
>  {
> - qxl_fbdev_fini(qdev);
> -
>   qxl_destroy_monitors_object(qdev);
>   drm_mode_config_cleanup(>ddev);
>  }
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 13c8a662f9..3fce7d16df 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -93,6 +93,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   if (ret)
>   goto modeset_cleanup;
>  
> + drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");

I couldn't find that this was part of old fbdev code, so it would be
nice to mention it in the commit message.

Acked-by: Noralf Trønnes 


> + drm_fbdev_generic_setup(>ddev, 32);
>   return 0;
>  
>  modeset_cleanup:
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 16/23] drm/qxl: implement prime kmap/kunmap

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Generic fbdev emulation needs this.  Also: We must keep track of the
> number of mappings now, so we don't unmap early in case two users want a
> kmap of the same bo.  Add a sanity check to destroy callback to make
> sure kmap/kunmap is balanced.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Just a note: You catch the one-to-many kmap type of unbalance, but not
the one-too-many kunmap situation.

Acked-by: Noralf Trønnes 


>  drivers/gpu/drm/qxl/qxl_drv.h|  1 +
>  drivers/gpu/drm/qxl/qxl_object.c |  6 ++
>  drivers/gpu/drm/qxl/qxl_prime.c  | 17 +
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 43c6df9cf9..8c3af1cdbe 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -84,6 +84,7 @@ struct qxl_bo {
>   struct ttm_bo_kmap_obj  kmap;
>   unsigned int pin_count;
>   void*kptr;
> + unsigned intmap_count;
>   int type;
>  
>   /* Constant after initialization */
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c 
> b/drivers/gpu/drm/qxl/qxl_object.c
> index 024c8dd317..4928fa6029 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -36,6 +36,7 @@ static void qxl_ttm_bo_destroy(struct ttm_buffer_object 
> *tbo)
>   qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;
>  
>   qxl_surface_evict(qdev, bo, false);
> + WARN_ON_ONCE(bo->map_count > 0);
>   mutex_lock(>gem.mutex);
>   list_del_init(>list);
>   mutex_unlock(>gem.mutex);
> @@ -131,6 +132,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>   if (bo->kptr) {
>   if (ptr)
>   *ptr = bo->kptr;
> + bo->map_count++;
>   return 0;
>   }
>   r = ttm_bo_kmap(>tbo, 0, bo->tbo.num_pages, >kmap);
> @@ -139,6 +141,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>   bo->kptr = ttm_kmap_obj_virtual(>kmap, _iomem);
>   if (ptr)
>   *ptr = bo->kptr;
> + bo->map_count = 1;
>   return 0;
>  }
>  
> @@ -180,6 +183,9 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
>  {
>   if (bo->kptr == NULL)
>   return;
> + bo->map_count--;
> + if (bo->map_count > 0)
> + return;
>   bo->kptr = NULL;
>   ttm_bo_kunmap(>kmap);
>  }
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index a55dece118..708378844c 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -22,7 +22,7 @@
>   * Authors: Andreas Pokorny
>   */
>  
> -#include "qxl_drv.h"
> +#include "qxl_object.h"
>  
>  /* Empty Implementations as there should not be any other driver for a 
> virtual
>   * device that might share buffers with qxl */
> @@ -54,13 +54,22 @@ struct drm_gem_object *qxl_gem_prime_import_sg_table(
>  
>  void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> - return ERR_PTR(-ENOSYS);
> + struct qxl_bo *bo = gem_to_qxl_bo(obj);
> + void *ptr;
> + int ret;
> +
> + ret = qxl_bo_kmap(bo, );
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + return ptr;
>  }
>  
>  void qxl_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>  {
> - WARN_ONCE(1, "not implemented");
> + struct qxl_bo *bo = gem_to_qxl_bo(obj);
> +
> + qxl_bo_kunmap(bo);
>  }
>  
>  int qxl_gem_prime_mmap(struct drm_gem_object *obj,
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 14/23] drm/qxl: cover all crtcs in shadow bo.

[Noralf Trønnes]

e(qdev, user_bo->gem_base.size,
> -   true, true, QXL_GEM_DOMAIN_SURFACE, NULL,
> -   _bo->shadow);
> - user_bo->shadow->surf = user_bo->surf;
> + if (user_bo->shadow != qdev->dumb_shadow_bo) {
> + if (user_bo->shadow) {
> + drm_gem_object_put_unlocked
> + (_bo->shadow->gem_base);
> + user_bo->shadow = NULL;
> + }
> + drm_gem_object_get(>dumb_shadow_bo->gem_base);
> + user_bo->shadow = qdev->dumb_shadow_bo;
>   }
>   }
>  
> @@ -773,7 +838,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
>   user_bo = gem_to_qxl_bo(obj);
>   qxl_bo_unpin(user_bo);
>  
> - if (user_bo->shadow && !user_bo->shadow->is_primary) {
> + if (old_state->fb != plane->state->fb && user_bo->shadow) {
>   drm_gem_object_put_unlocked(_bo->shadow->gem_base);
>   user_bo->shadow = NULL;
>   }
> @@ -1106,6 +1171,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  
>   memset(qdev->monitors_config, 0, monitors_config_size);
>   qdev->monitors_config->max_allowed = max_allowed;
> +
> + qdev->dumb_heads = kcalloc(max_allowed, sizeof(qdev->dumb_heads[0]), 
> GFP_KERNEL);

Needs an allocation failure check. With that:

Acked-by: Noralf Trønnes 


>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
> index c408bb83c7..5313ad21c1 100644
> --- a/drivers/gpu/drm/qxl/qxl_draw.c
> +++ b/drivers/gpu/drm/qxl/qxl_draw.c
> @@ -267,7 +267,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>  struct qxl_bo *bo,
>  unsigned int flags, unsigned int color,
>  struct drm_clip_rect *clips,
> -unsigned int num_clips, int inc)
> +unsigned int num_clips, int inc,
> +uint32_t dumb_shadow_offset)
>  {
>   /*
>* TODO: if flags & DRM_MODE_FB_DIRTY_ANNOTATE_FILL then we should
> @@ -295,6 +296,9 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   if (ret)
>   return;
>  
> + clips->x1 += dumb_shadow_offset;
> + clips->x2 += dumb_shadow_offset;
> +
>   left = clips->x1;
>   right = clips->x2;
>   top = clips->y1;
> @@ -342,7 +346,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   goto out_release_backoff;
>  
>   ret = qxl_image_init(qdev, release, dimage, surface_base,
> -  left, top, width, height, depth, stride);
> +  left - dumb_shadow_offset,
> +  top, width, height, depth, stride);
>   qxl_bo_kunmap(bo);
>   if (ret)
>   goto out_release_backoff;
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 13/23] drm/qxl: use shadow bo directly

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Pass the shadow bo to qxl_io_create_primary() instead of expecting
> qxl_io_create_primary to check bo->shadow.  Set is_primary flag on the
> shadow bo.  Move the is_primary tracking into qxl_io_create_primary()
> and qxl_io_destroy_primary() functions.
> 
> That simplifies primary surface tracking and the workflow in
> qxl_primary_atomic_update().
> 
> Signed-off-by: Gerd Hoffmann 
> 
> qxl_io_create/destroy_primary: primary_bo tracking [fixup]
> ---

I'm unable to follow the logic, but I didn't spot anything suspicious
looking.

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 12/23] drm/qxl: track primary bo

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Track which bo is used as primary surface.  With that in place we don't
> need the primary_created flag any more, we can just check the primary bo
> pointer instead.
> 
> Also verify we don't already have a primary surface in
> qxl_io_create_primary().
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 11/23] drm/qxl: drop unused offset parameter from qxl_io_create_primary()

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 10/23] drm/qxl: move qxl_primary_apply_cursor to correct place

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> The cursor must be set again after creating the primary surface.
> Also drop the error message.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 86bfc19bea..1b700ef503 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane 
> *plane,
>   .x2 = plane->state->fb->width,
>   .y2 = plane->state->fb->height
>   };
> - int ret;
>   bool same_shadow = false;
>  
>   if (old_state->fb) {
> @@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane 
> *plane,
>   if (!same_shadow)
>   qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
> -
> - ret = qxl_primary_apply_cursor(plane);
> - if (ret)
> - DRM_ERROR(
> - "could not set cursor after creating primary");
>   }
>  
>   if (!bo->is_primary) {
> - if (!same_shadow)
> + if (!same_shadow) {
>   qxl_io_create_primary(qdev, 0, bo);
> + qxl_primary_apply_cursor(plane);
> + }
>   bo->is_primary = true;
>   }
>  
> 

I don't see how the commit message matches what you're doing. It gives
the impression that it must be applied under yet another condition, but
the condition for applying the cursor is changed from bo_old->is_primary
to !bo->is_primary.
It probably makes sense to someone that knows the driver.

Acked-by: Noralf Trønnes 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 14/15] drm/bochs: drop old fbdev emulation code

[Noralf Trønnes]

Den 10.01.2019 11.16, skrev Gerd Hoffmann:
>   Hi,
> 
>>> -   drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);
>>
>> The generic fbdev emulation doesn't take care of suspend/resume. You
>> could do this:
>> drm_fb_helper_set_suspend_unlocked(drm_dev->fb_helper, 1);
> 
> Additional to drm_mode_config_helper_suspend() I assume?

No, sorry for the confusion. I didn't know if you could use
drm_mode_config_helper_suspend() or not. That function takes care of
fbdev suspend as well:

int drm_mode_config_helper_suspend(struct drm_device *dev)
{
<...>
drm_kms_helper_poll_disable(dev);
drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
state = drm_atomic_helper_suspend(dev);
<...>
}


> Does the call order matter?
> 
>>> drm_helper_resume_force_mode(drm_dev);
>>
>> The docs for this function has this to say:
>> * This function is deprecated. New drivers should implement atomic mode-
>> * setting and use the atomic suspend/resume helpers.
>>
>> Maybe you can use drm_mode_config_helper_suspend/resume() instead like
>> suggested in the drm_fbdev_generic_setup() docs.
> 
> That should work, yes.  The driver is simple enough ;)
> 
> cheers,
>   Gerd
> 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 14/15] drm/bochs: drop old fbdev emulation code

[Noralf Trønnes]

Den 10.01.2019 09.28, skrev Gerd Hoffmann:
> Not needed any more, bochs uses the generic emulation now.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Oleksandr Andrushchenko 
> Acked-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/bochs/bochs.h   |   9 ---
>  drivers/gpu/drm/bochs/bochs_drv.c   |   6 --
>  drivers/gpu/drm/bochs/bochs_fbdev.c | 129 
> 
>  3 files changed, 144 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> index ede22beb85..03711394f1 100644
> --- a/drivers/gpu/drm/bochs/bochs.h
> +++ b/drivers/gpu/drm/bochs/bochs.h
> @@ -80,12 +80,6 @@ struct bochs_device {
>   struct ttm_bo_device bdev;
>   bool initialized;
>   } ttm;
> -
> - /* fbdev */
> - struct {
> - struct drm_framebuffer *fb;
> - struct drm_fb_helper helper;
> - } fb;
>  };
>  
>  struct bochs_bo {
> @@ -157,7 +151,4 @@ int bochs_kms_init(struct bochs_device *bochs);
>  void bochs_kms_fini(struct bochs_device *bochs);
>  
>  /* bochs_fbdev.c */
> -int bochs_fbdev_init(struct bochs_device *bochs);
> -void bochs_fbdev_fini(struct bochs_device *bochs);
> -
>  extern const struct drm_mode_config_funcs bochs_mode_funcs;
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
> b/drivers/gpu/drm/bochs/bochs_drv.c
> index 1d86c0fb5f..a8968cd8d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -102,12 +102,9 @@ static int bochs_pm_suspend(struct device *dev)
>  {
>   struct pci_dev *pdev = to_pci_dev(dev);
>   struct drm_device *drm_dev = pci_get_drvdata(pdev);
> - struct bochs_device *bochs = drm_dev->dev_private;
>  
>   drm_kms_helper_poll_disable(drm_dev);
>  
> - drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);

The generic fbdev emulation doesn't take care of suspend/resume. You
could do this:
drm_fb_helper_set_suspend_unlocked(drm_dev->fb_helper, 1);

> -
>   return 0;
>  }
>  
> @@ -115,12 +112,9 @@ static int bochs_pm_resume(struct device *dev)
>  {
>   struct pci_dev *pdev = to_pci_dev(dev);
>   struct drm_device *drm_dev = pci_get_drvdata(pdev);
> - struct bochs_device *bochs = drm_dev->dev_private;
>  
>   drm_helper_resume_force_mode(drm_dev);

The docs for this function has this to say:
* This function is deprecated. New drivers should implement atomic mode-
* setting and use the atomic suspend/resume helpers.

Maybe you can use drm_mode_config_helper_suspend/resume() instead like
suggested in the drm_fbdev_generic_setup() docs.

Noralf.

>  
> - drm_fb_helper_set_suspend_unlocked(>fb.helper, 0);
> -
>   drm_kms_helper_poll_enable(drm_dev);
>   return 0;
>  }
> diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
> b/drivers/gpu/drm/bochs/bochs_fbdev.c
> index ccf783b038..7cac3f5253 100644
> --- a/drivers/gpu/drm/bochs/bochs_fbdev.c
> +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
> @@ -11,124 +11,6 @@
>  
>  /* -- */
>  
> -static int bochsfb_mmap(struct fb_info *info,
> - struct vm_area_struct *vma)
> -{
> - struct drm_fb_helper *fb_helper = info->par;
> - struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
> -
> - return ttm_fbdev_mmap(vma, >bo);
> -}
> -
> -static struct fb_ops bochsfb_ops = {
> - .owner = THIS_MODULE,
> - DRM_FB_HELPER_DEFAULT_OPS,
> - .fb_fillrect = drm_fb_helper_cfb_fillrect,
> - .fb_copyarea = drm_fb_helper_cfb_copyarea,
> - .fb_imageblit = drm_fb_helper_cfb_imageblit,
> - .fb_mmap = bochsfb_mmap,
> -};
> -
> -static int bochsfb_create_object(struct bochs_device *bochs,
> -  const struct drm_mode_fb_cmd2 *mode_cmd,
> -  struct drm_gem_object **gobj_p)
> -{
> - struct drm_device *dev = bochs->dev;
> - struct drm_gem_object *gobj;
> - u32 size;
> - int ret = 0;
> -
> - size = mode_cmd->pitches[0] * mode_cmd->height;
> - ret = bochs_gem_create(dev, size, true, );
> - if (ret)
> - return ret;
> -
> - *gobj_p = gobj;
> - return ret;
> -}
> -
> -static int bochsfb_create(struct drm_fb_helper *helper,
> -   struct drm_fb_helper_surface_size *sizes)
> -{
> - struct bochs_device *bochs =
> - container_of(helper, struct bochs_device, fb.helper);
> - struct fb_info *info;
> - struct drm_framebuffer *fb;
> - struct drm_mode_fb_cmd2 mode_cmd;
> - struct drm_gem_object *gobj = NULL;
> - struct bochs_bo *bo = NULL;
> - int size, ret;
> -
> - if (sizes->surface_bpp != 32)
> - return -EINVAL;
> -
> - mode_cmd.width = sizes->surface_width;
> - mode_cmd.height = sizes->surface_height;
> - mode_cmd.pitches[0] = sizes->surface_width * 4;
> - mode_cmd.pixel_format = DRM_FORMAT_HOST_XRGB;
> - size = mode_cmd.pitches[0] * mode_cmd.height;
> -
> - /* 

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 19.49, skrev Ville Syrjälä:

On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote:

tinydrm is also using plane->fb:

$ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
drivers/gpu/drm/tinydrm/repaper.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb =
mipi->tinydrm.pipe.plane.fb;

Oh dear, and naturally it's the most annoying one of the bunch. So we
want to take the plane lock in the fb.dirty() hook to look at the fb,
but mipi-dbi.c calls that directly from the bowels of its
.atomic_enable() hook. So that would deadlock pretty neatly if the
commit happens while already holding the lock.

So we'd either need to plumb an acquire context into fb.dirty(),
or maybe have tinydrm provide a lower level lockless dirty() hook
that gets called by both (there are just too many layers in this
particular cake to immediately see where such a hook would be best
placed). Or maybe there's some other solution I didn't think of.

Looking at this I'm also left wondering how this is supposed
handle fb.dirty() getting called concurrently with a modeset.
The dirty_mutex only seems to offer any protection for
fb.dirty() against another fb.dirty()...



I have been waiting for the 'page-flip with damage'[1] work to come to
fruition so I could handle all flushing during atomic commit.
The idea is to use the same damage rect for a generic dirtyfb callback.

Maybe a temporary tinydrm specific solution is possible until that work
lands, but I don't know enough about how to implement such a dirtyfb
callback.

I imagine it could look something like this:

 struct tinydrm_device {
+    struct drm_clip_rect dirtyfb_rect;
 };

static int tinydrm_fb_dirty(struct drm_framebuffer *fb,
                struct drm_file *file_priv,
                unsigned int flags, unsigned int color,
                struct drm_clip_rect *clips,
                unsigned int num_clips)
{
    struct tinydrm_device *tdev = fb->dev->dev_private;
    struct drm_framebuffer *planefb;

    /* Grab whatever lock needed to check this */
    planefb = tdev->pipe.plane.state->fb;

    /* fbdev can flush even when we're not interested */
    if (fb != planefb)
        return 0;

    /* Protect dirtyfb_rect with a lock */
    tinydrm_merge_clips(>dirty_rectfb, clips, num_clips, flags,
                fb->width, fb->height);

    /*
     * Somehow do an atomic commit that results in the atomic update
     * callback being called
     */
    ret = drm_atomic_commit(state);
...
}

static void mipi_dbi_flush(struct drm_framebuffer *fb,
               struct drm_clip_rect *clip)
{
    /* Flush out framebuffer */
}

void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
              struct drm_plane_state *old_state)
{
    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
    struct drm_framebuffer *fb = pipe->plane.state->fb;
    struct drm_crtc *crtc = >pipe.crtc;

    if (!fb || (fb == old_state->fb && !tdev->dirty_rect.x2))
        goto out_vblank;

    /* Don't flush if the controller isn't initialized yet */
    if (!mipi->enabled)
        goto out_vblank;

    if (fb != old_state->fb) { /* Page flip or new, flush all */
        mipi_dbi_flush(fb, NULL);
    } else { /* dirtyfb ioctl */
        mipi_dbi_flush(fb, >dirtyfb_rect);
        memset(>dirtyfb_rect, 0, sizeof(tdev->dirtyfb_rect));
    }

out_vblank:
    if (crtc->state->event) {
        spin_lock_irq(>dev->event_lock);
        drm_crtc_send_vblank_event(crtc, crtc->state->event);
        spin_unlock_irq(>dev->event_lock);
        crtc->state->event = NULL;
    }
}

This is called from the driver pipe enable callback after the controller
has been initialized:

 void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
           struct drm_crtc_state *crtc_state)
 {
 struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
 struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-     struct drm_framebuffer *fb = pipe->plane.fb;
+    struct drm_framebuffer *fb = pipe->plane.state->fb;

 DRM_DEBUG_KMS("\n");

 mipi->enabled = true;
-     if (fb)
-         fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+    mipi_dbi_flush(fb, NULL);
 tinydrm_enable_backlight(mipi->backlight);
 }

I can make patches if this makes any sense and you can help me with the
dirtyfb implementation.

Noralf.

[1] 
https://lists.freedesktop.org/archives/dri-devel/2017-December/161003.html


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 16.22, skrev Ville Syrjala:

From: Ville Syrjälä 

I really just wanted to fix i915 to re-enable its planes afer load
detection (a two line patch). This is what I actually ended up with
after I ran into a framebuffer refcount leak with said two line patch.

I've tested this on a few i915 boxes and so far it's looking
good. Everything else is just compile tested.

Entire series available here:
git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke

Cc: Alex Deucher 
Cc: amd-...@lists.freedesktop.org
Cc: Benjamin Gaignard 
Cc: Boris Brezillon 
Cc: ch...@chris-wilson.co.uk
Cc: "Christian König" 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: David Airlie 
Cc: "David (ChunMing) Zhou" 
Cc: Eric Anholt 
Cc: freedr...@lists.freedesktop.org
Cc: Gerd Hoffmann 
Cc: Harry Wentland 
Cc: Inki Dae 
Cc: Joonyoung Shim 
Cc: Kyungmin Park 
Cc: linux-arm-...@vger.kernel.org
Cc: Maarten Lankhorst 
Cc: martin.pe...@free.fr
Cc: Rob Clark 
Cc: Seung-Woo Kim 
Cc: Shawn Guo 
Cc: Sinclair Yeh 
Cc: Thomas Hellstrom 
Cc: Vincent Abriou 
Cc: virtualization@lists.linux-foundation.org
Cc: VMware Graphics 

Ville Syrjälä (23):
   Revert "drm/atomic-helper: Fix leak in disable_all"
   drm/atomic-helper: Make drm_atomic_helper_disable_all() update the
 plane->fb pointers
   drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc()
   drm/atomic-helper: WARN if legacy plane fb pointers are bogus when
 committing duplicated state
   drm: Add local 'plane' variable for primary/cursor planes
   drm: Adjust whitespace for legibility
   drm: Make the fb refcount handover less magic
   drm: Use plane->state->fb over plane->fb
   drm/i915: Stop consulting plane->fb
   drm/msm: Stop consulting plane->fb
   drm/sti: Stop consulting plane->fb
   drm/vmwgfx: Stop consulting plane->fb
   drm/zte: Stop consulting plane->fb
   drm/atmel-hlcdc: Stop using plane->fb
   drm: Stop updating plane->crtc/fb/old_fb on atomic drivers
   drm/amdgpu/dc: Stop updating plane->fb
   drm/i915: Stop updating plane->fb/crtc
   drm/exynos: Stop updating plane->crtc
   drm/msm: Stop updating plane->fb/crtc
   drm/virtio: Stop updating plane->fb
   drm/vc4: Stop updating plane->fb/crtc
   drm/i915: Restore planes after load detection
   drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug


tinydrm is also using plane->fb:

$ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
drivers/gpu/drm/tinydrm/repaper.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = 
mipi->tinydrm.pipe.plane.fb;

drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c: pipe->plane.fb = fb;
drivers/gpu/drm/tinydrm/ili9225.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/st7586.c:   if (tdev->pipe.plane.fb != fb)

Noralf.


  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 -
  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 12 +---
  drivers/gpu/drm/drm_atomic.c  | 55 ++--
  drivers/gpu/drm/drm_atomic_helper.c   | 79 ++-
  drivers/gpu/drm/drm_crtc.c| 51 ++-
  drivers/gpu/drm/drm_fb_helper.c   |  7 --
  drivers/gpu/drm/drm_framebuffer.c |  5 --
  drivers/gpu/drm/drm_plane.c   | 64 +++---
  drivers/gpu/drm/drm_plane_helper.c|  4 +-
  drivers/gpu/drm/exynos/exynos_drm_plane.c |  2 -
  drivers/gpu/drm/i915/intel_crt.c  |  6 ++
  drivers/gpu/drm/i915/intel_display.c  |  9 +--
  drivers/gpu/drm/i915/intel_fbdev.c|  2 +-
  drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c |  3 +-
  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c|  2 -
  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  2 -
  drivers/gpu/drm/sti/sti_plane.c   |  9 +--
  drivers/gpu/drm/vc4/vc4_crtc.c|  3 -
  drivers/gpu/drm/virtio/virtgpu_display.c  |  2 -
  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |  6 +-
  drivers/gpu/drm/zte/zx_vou.c  |  2 +-
  include/drm/drm_atomic.h  |  3 -
  22 files changed, 143 insertions(+), 187 deletions(-)



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org

Re: [PATCH libdrm] drm: Remove create_handle() drm_framebuffer "virtual".

[Noralf Trønnes]

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

[...]


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.

Noralf.


const struct drm_framebuffer_funcs *fb_funcs;
  };
  
@@ -72,7 +67,6 @@ struct drm_fbdev_cma {

   *
   * static struct drm_framebuffer_funcs driver_fb_funcs = {
   * .destroy   = drm_fb_cma_destroy,
- * .create_handle = drm_fb_cma_create_handle,
   * .dirty = driver_fb_dirty,
   * };
   *
@@ -90,67 +84,50 @@ static inline struct drm_fbdev_cma *to_fbdev_cma(struct 
drm_fb_helper *helper)
return container_of(helper, struct drm_fbdev_cma, fb_helper);
  }
  
-static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)

-{
-   return container_of(fb, struct drm_fb_cma, fb);
-}
-
  void drm_fb_cma_destroy(struct drm_framebuffer *fb)
  {
-   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
int i;
  
  	for (i = 0; i < 4; i++) {

-   if (fb_cma->obj[i])
-   drm_gem_object_put_unlocked(_cma->obj[i]->base);
+   if (fb->gem_objs[i])
+   drm_gem_object_put_unlocked(fb->gem_objs[i]);
}
  
  	drm_framebuffer_cleanup(fb);

-   kfree(fb_cma);
+   kfree(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)
-{
-   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
-
-   return drm_gem_handle_create(file_priv,
-   _cma->obj[0]->base, handle);
-}
-EXPORT_SYMBOL(drm_fb_cma_create_handle);
-
  static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
.destroy= drm_fb_cma_destroy,
-   .create_handle  = drm_fb_cma_create_handle,
  };
  
-static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,

+static struct drm_framebuffer *drm_fb_cma_alloc(struct drm_device *dev,
const struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_gem_cma_object **obj,
unsigned int num_planes, const struct drm_framebuffer_funcs *funcs)
  {
-   struct drm_fb_cma *fb_cma;
+   struct drm_framebuffer *fb;
int ret;
int i;
  
-	fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);

-   if (!fb_cma)
+   fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+   if (!fb)
return ERR_PTR(-ENOMEM);
  
-	drm_helper_mode_fill_fb_struct(dev, _cma->fb, mode_cmd);

+   drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
  
  	for (i = 0; i < num_planes; i++)

-   fb_cma->obj[i] = obj[i];
+   fb->gem_objs[i] = [i]->base;
  
-	ret = drm_framebuffer_init(dev, _cma->fb, funcs);

+   ret = drm_framebuffer_init(dev, fb, funcs);
if (ret) {
dev_err(dev->dev, "Failed to initialize framebuffer: %d\n", 
ret);
-   kfree(fb_cma);
+   kfree(fb);
return ERR_PTR(ret);
}
  
-	return fb_cma;

+   return fb;
  }
  
  /**

@@ -171,7 +148,7 @@ struct drm_framebuffer *drm_fb_cma_create_with_funcs(struct 
drm_device *dev,
const struct drm_framebuffer_funcs *funcs)
  {
const struct drm_format_info *info;
-   struct drm_fb_cma *fb_cma;
+   struct drm_framebuffer *fb;
struct drm_gem_cma_object *objs[4];
struct drm_gem_object *obj;
int ret;
@@ -205,13 +182,13 @@ struct drm_framebuffer 
*drm_fb_cma_create_with_funcs(struct drm_device *dev,
objs[i] = to_drm_gem_cma_obj(obj);
}
  
-	fb_cma = drm_fb_cma_alloc(dev, mode_cmd, objs, i, funcs);

-   if (IS_ERR(fb_cma)) {
-   ret = 

Re: [PATCH 7/8] drm: Nuke drm_atomic_helper_connector_dpms

[Noralf Trønnes]

Den 25.07.2017 10.01, skrev Daniel Vetter:

It's dead code, the core handles all this directly now.

The only special case is nouveau and tda988x which used one function
for both legacy modeset code and -nv50 atomic world instead of 2
vtables. But amounts to exactly the same.

v2: Rebase over the panel/brideg refactorings in stm/ltdc.

Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>

...

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c 
b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
index ec43fb7ad9e4..79b6687977d3 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
@@ -71,7 +71,6 @@ static void tinydrm_connector_destroy(struct drm_connector 
*connector)
  }
  
  static const struct drm_connector_funcs tinydrm_connector_funcs = {

-   .dpms = drm_atomic_helper_connector_dpms,
.reset = drm_atomic_helper_connector_reset,
.detect = tinydrm_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,


Acked-by: Noralf Trønnes <nor...@tronnes.org>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization