nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

Dear,

I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel 
code, git drm 2.4.36. 
  When I run the vbltest program, it prints  "60HZ"  which indicated the 
implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
  But when I run modetest with option " -v -s 12:1280x1024" , it prints high 
fresh rate up to "150 HZ" .  I examing the code , and found that no waiting 
vblank operation is processed in nouveau_crtc_page_flip() function. The screen  
produced lots of garbage and blink very much.
 int
nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
   struct drm_pending_vblank_event *event)
{
..
}
I study the i915 intel_crtc_page_flip implementation.
static int intel_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)
{
..

ret = drm_vblank_get(dev, intel_crtc->pipe);
if (ret)
goto cleanup_objs;

work->pending_flip_obj = obj;

work->enable_stall_check = true;

/* Block clients from rendering to the new back buffer until
 * the flip occurs and the object is no longer visible.
 */
atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);

ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
if (ret)
goto cleanup_pending;
..
}

after vblank irq acquired, the interrupt isr will wakup the runqueue.
6159 static void do_intel_finish_page_flip(struct drm_device *dev,
6160   struct drm_crtc *crtc)
6161 {
..
6211 list_add_tail(&e->base.link,
6212   &e->base.file_priv->event_list);
6213 wake_up_interruptible(&e->base.file_priv->event_wait);
6214 }
6215
6216 drm_vblank_put(dev, intel_crtc->pipe);
6217


Is there anyone use the same  driver and found this issues can tell me "is it a 
bug"?

Thanks!


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

Re:nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

Can anyone give a suggestion, is wait-vblank fully implemented in page_flip() 
for nouveau drm driver?



At 2011-10-24 14:30:55,chris  wrote:

Dear,

I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel 
code, git drm 2.4.36. 
  When I run the vbltest program, it prints  "60HZ"  which indicated the 
implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
  But when I run modetest with option " -v -s 12:1280x1024" , it prints high 
fresh rate up to "150 HZ" .  I examing the code , and found that no waiting 
vblank operation is processed in nouveau_crtc_page_flip() function. The screen  
produced lots of garbage and blink very much.
 int
nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
   struct drm_pending_vblank_event *event)
{
..
}
I study the i915 intel_crtc_page_flip implementation.
static int intel_crtc_page_flip(stru ct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)
{
..

ret = drm_vblank_get(dev, intel_crtc->pipe);
if (ret)
goto cleanup_objs;

work->pending_flip_obj = obj;

work->enable_stall_check = true;

/* Block clients from rendering to the new back buffer until
 * the flip occurs and the object is no longer visible.
 */
atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);

ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
 &nb sp;  if (ret)
goto cleanup_pending;
..
}

after vblank irq acquired, the interrupt isr will wakup the runqueue.
6159 static void do_intel_finish_page_flip(struct drm_device *dev,
6160   struct drm_crtc *crtc)
6161 {
..
6211 list_add_tail(&e->base.link,
6212   &e->base.file_priv->event_list);
6213 wake_up_interruptible(&e->base.file_priv->event_wait);
6214 }
6215
6216 drm_vblank_put(dev, intel_crtc->pipe);
6217


Is there anyone use the same  driver and foun d this issues can tell me "is it 
a bug"?

Thanks!





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

Re:Re: nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

It seems that nv04_graph_mthd_page_flip() was called from isr , and  page_flip 
ioctl only send a NV_SW_PAGE_FLIP  pushbuffer bo to gem. If you don't want the 
screen show garbage you must make sure not to change frontbuffer or backbuffer 
in GPU when nv_set_crtc_base()  is called, and not  change the frontbuffer 
context when it is rendering, am I right?
At 2011-10-25 13:45:29,"Maarten Maathuis"  wrote:
>2011/10/25 chris :
>> Can anyone give a suggestion, is wait-vblank fully implemented in
>> page_flip() for nouveau drm driver?
>>
>>
>> At 2011-10-24 14:30:55,chris  wrote:
>>
>> Dear,
>>
>> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel
>> code, git drm 2.4.36.
>>   When I run the vbltest program, it prints  "60HZ"  which indicated the
>> implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
>>   But when I run modetest with option " -v -s 12:1280x1024" , it prints high
>> fresh rate up to "150 HZ" .  I examing the code , and found that no waiting
>> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>> screen  produced lots of garbage and blink very much.
>>  int
>> nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>>    struct drm_pending_vblank_event *event)
>> {
>> ..
>> }
>> I study the i915 intel_crtc_page_flip implementation.
>> static int intel_crtc_page_flip(stru ct drm_crtc *crtc,
>>     struct drm_framebuffer *fb,
>>     struct drm_pending_vblank_event *event)
>> {
>> ..
>>
>>     ret = drm_vblank_get(dev, intel_crtc->pipe);
>>     if (ret)
>>     goto cleanup_objs;
>>
>>     work->pending_flip_obj = obj;
>>
>>     work->enable_stall_check = true;
>>
>>     /* Block clients from rendering to the new back buffer until
>>  * the flip occurs and the object is no longer visible.
>>  */
>>     atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);
>>
>>     ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
>>  &am p;nb sp;  if (ret)
>>     goto cleanup_pending;
>> ..
>> }
>>
>> after vblank irq acquired, the interrupt isr will wakup the runqueue.
>> 6159 static void do_intel_finish_page_flip(struct drm_device *dev,
>> 6160   struct drm_crtc *crtc)
>> 6161 {
>> ..
>> 6211 list_add_tail(&e->base.link,
>> 6212   &e->base.file_priv->event_list);
>> 6213 wake_up_interruptible(&e->base.file_priv->event_wait);
>> 6214 }
>> 6215
>> 6216 drm_vblank_put(dev, intel_crtc->pipe);
>> 6217
>>
>>
>> Is there anyone use the same  driver and foun d this issues can tell me "is
>> it a bug"?
>>
>> Thanks!
>>
>>
>>
>>
>>
>>
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>
>It seems to be, the actual page flipping is done by software method
>(see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
>and that is that we wait for the rendering to be done to the current
>frontbuffer and not the current backbuffer (this is only done if the
>page flip channel is different than the rendering channel). Maybe
>someone else can comment on that.
>
>-- 
>Far away from the primal instinct, the song seems to fade away, the
>river get wider between your thoughts and the things we do and say.

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

Re:Re: nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

I think page_flip ioctl need to realize a synchronous mechanism to control 
fresh rate...!!!
At 2011-10-25 20:30:39,"Ben Skeggs"  wrote:
>On Tue, 2011-10-25 at 14:15 +0200, Francisco Jerez wrote:
>> Maarten Maathuis  writes:
>> 
>> > 2011/10/25 chris :
>> >> Can anyone give a suggestion, is wait-vblank fully implemented in
>> >> page_flip() for nouveau drm driver?
>> >>
>> 
>> It's intentionally not implemented.  The reason is that I wanted to
>> support non-vsync'ed vblank as well, and for vsync'ed blits we had to
>> think about a different mechanism for vblank synchronization anyway, so
>> I figured it didn't make that much sense to force vblank synchronization
>> directly from the pageflip ioctl.
>+1 I deliberately didn't flip 1 bit in the NV50/NVC0 page flipping code
>for this as well.  The interface IMO is flawed.  Though, that said, we
>really should look at doing something properly for this, a lot of people
>do want tear-free goodness.
>
>Ben.
>> 
>> >>
>> >> At 2011-10-24 14:30:55,chris  wrote:
>> >>
>> >> Dear,
>> >>
>> >> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 
>> >> kernel
>> >> code, git drm 2.4.36.
>> >>   When I run the vbltest program, it prints  "60HZ"  which indicated the
>> >> implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
>> >>   But when I run modetest with option " -v -s 12:1280x1024" , it prints 
>> >> high
>> >> fresh rate up to "150 HZ" .  I examing the code , and found that no 
>> >> waiting
>> >> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>> >> screen  produced lots of garbage and blink very much.
>> 
>> That's fine if by "garbage" you just mean it's tearing like crazy.
>> 
>> >>[...]
>> >
>> > It seems to be, the actual page flipping is done by software method
>> > (see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
>> > and that is that we wait for the rendering to be done to the current
>> > frontbuffer and not the current backbuffer (this is only done if the
>> > page flip channel is different than the rendering channel). Maybe
>> > someone else can comment on that.
>> 
>> There's no need to wait for the backbuffer rendering to end because the
>> pageflip is always pushed through the last channel that has queued
>> rendering to it, so, the "waiting" is actually done by the GPU. The
>> waiting to the current frontbuffer (which in most cases is going to be a
>> cross-channel barrier instead of actual CPU waiting) is necessary for
>> the (rare) case where you have several channels trying to render to the
>> same pageflipped drawable, to make sure that the flips are properly
>> synchronized with respect each other.
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>___
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

Dear,

I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel 
code, git drm 2.4.36. 
  When I run the vbltest program, it prints  "60HZ"  which indicated the 
implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
  But when I run modetest with option " -v -s 12:1280x1024" , it prints high 
fresh rate up to "150 HZ" .  I examing the code , and found that no waiting 
vblank operation is processed in nouveau_crtc_page_flip() function. The screen  
produced lots of garbage and blink very much.
 int
nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
   struct drm_pending_vblank_event *event)
{
..
}
I study the i915 intel_crtc_page_flip implementation.
static int intel_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)
{
..

ret = drm_vblank_get(dev, intel_crtc->pipe);
if (ret)
goto cleanup_objs;

work->pending_flip_obj = obj;

work->enable_stall_check = true;

/* Block clients from rendering to the new back buffer until
 * the flip occurs and the object is no longer visible.
 */
atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);

ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
if (ret)
goto cleanup_pending;
..
}

after vblank irq acquired, the interrupt isr will wakup the runqueue.
6159 static void do_intel_finish_page_flip(struct drm_device *dev,
6160   struct drm_crtc *crtc)
6161 {
..
6211 list_add_tail(&e->base.link,
6212   &e->base.file_priv->event_list);
6213 wake_up_interruptible(&e->base.file_priv->event_wait);
6214 }
6215
6216 drm_vblank_put(dev, intel_crtc->pipe);
6217


Is there anyone use the same  driver and found this issues can tell me "is it a 
bug"?

Thanks!


-- next part --
An HTML attachment was scrubbed...
URL: 

nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

Can anyone give a suggestion, is wait-vblank fully implemented in page_flip() 
for nouveau drm driver?



At 2011-10-24 14:30:55,chris  wrote:

Dear,

I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel 
code, git drm 2.4.36. 
  When I run the vbltest program, it prints  "60HZ"  which indicated the 
implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
  But when I run modetest with option " -v -s 12:1280x1024" , it prints high 
fresh rate up to "150 HZ" .  I examing the code , and found that no waiting 
vblank operation is processed in nouveau_crtc_page_flip() function. The screen  
produced lots of garbage and blink very much.
 int
nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
   struct drm_pending_vblank_event *event)
{
..
}
I study the i915 intel_crtc_page_flip implementation.
static int intel_crtc_page_flip(stru ct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)
{
..

ret = drm_vblank_get(dev, intel_crtc->pipe);
if (ret)
goto cleanup_objs;

work->pending_flip_obj = obj;

work->enable_stall_check = true;

/* Block clients from rendering to the new back buffer until
 * the flip occurs and the object is no longer visible.
 */
atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);

ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
 &nb sp;  if (ret)
goto cleanup_pending;
..
}

after vblank irq acquired, the interrupt isr will wakup the runqueue.
6159 static void do_intel_finish_page_flip(struct drm_device *dev,
6160   struct drm_crtc *crtc)
6161 {
..
6211 list_add_tail(&e->base.link,
6212   &e->base.file_priv->event_list);
6213 wake_up_interruptible(&e->base.file_priv->event_wait);
6214 }
6215
6216 drm_vblank_put(dev, intel_crtc->pipe);
6217


Is there anyone use the same  driver and foun d this issues can tell me "is it 
a bug"?

Thanks!





-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20111025/d214d3c9/attachment.html>

nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

It seems that nv04_graph_mthd_page_flip() was called from isr , and  page_flip 
ioctl only send a NV_SW_PAGE_FLIP  pushbuffer bo to gem. If you don't want the 
screen show garbage you must make sure not to change frontbuffer or backbuffer 
in GPU when nv_set_crtc_base()  is called, and not  change the frontbuffer 
context when it is rendering, am I right?
At 2011-10-25 13:45:29,"Maarten Maathuis"  wrote:
>2011/10/25 chris :
>> Can anyone give a suggestion, is wait-vblank fully implemented in
>> page_flip() for nouveau drm driver?
>>
>>
>> At 2011-10-24 14:30:55,chris? wrote:
>>
>> Dear,
>>
>> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 kernel
>> code, git drm 2.4.36.
>> ? When I run the vbltest program, it prints? "60HZ"? which indicated the
>> implementation of drmWaitVBlank() and drm_vblank_wait()? is correct.
>> ? But when I run modetest with option " -v -s 12:1280x1024" , it prints high
>> fresh rate up to "150 HZ" .? I examing the code , and found that no waiting
>> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>> screen? produced lots of garbage and blink very much.
>> ?int
>> nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>> ?? struct drm_pending_vblank_event *event)
>> {
>> ..
>> }
>> I study the i915 intel_crtc_page_flip implementation.
>> static int intel_crtc_page_flip(stru ct drm_crtc *crtc,
>> ??? struct drm_framebuffer *fb,
>> ??? struct drm_pending_vblank_event *event)
>> {
>> ..
>>
>> ??? ret = drm_vblank_get(dev, intel_crtc->pipe);
>> ??? if (ret)
>> ??? goto cleanup_objs;
>>
>> ??? work->pending_flip_obj = obj;
>>
>> ??? work->enable_stall_check = true;
>>
>> ??? /* Block clients from rendering to the new back buffer until
>>  * the flip occurs and the object is no longer visible.
>>  */
>> ??? atomic_add(1 << intel_crtc->plane, &work->old_fb_obj->pending_flip);
>>
>> ??? ret = dev_priv->display.queue_flip(dev, crtc, fb, obj);
>> ?&am p;nb sp;? if (ret)
>> ??? goto cleanup_pending;
>> ..
>> }
>>
>> after vblank irq acquired, the interrupt isr will wakup the runqueue.
>> 6159 static void do_intel_finish_page_flip(struct drm_device *dev,
>> 6160?? struct drm_crtc *crtc)
>> 6161 {
>> ..
>> 6211 list_add_tail(&e->base.link,
>> 6212?? &e->base.file_priv->event_list);
>> 6213 wake_up_interruptible(&e->base.file_priv->event_wait);
>> 6214 }
>> 6215
>> 6216 drm_vblank_put(dev, intel_crtc->pipe);
>> 6217
>>
>>
>> Is there anyone use the same? driver and foun d this issues can tell me "is
>> it a bug"?
>>
>> Thanks!
>>
>>
>>
>>
>>
>>
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>
>It seems to be, the actual page flipping is done by software method
>(see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
>and that is that we wait for the rendering to be done to the current
>frontbuffer and not the current backbuffer (this is only done if the
>page flip channel is different than the rendering channel). Maybe
>someone else can comment on that.
>
>-- 
>Far away from the primal instinct, the song seems to fade away, the
>river get wider between your thoughts and the things we do and say.

nouveau page_flip function implement not wait vblank, which cause screen garbage

[chris]

I think page_flip ioctl need to realize a synchronous mechanism to control 
fresh rate...!!!
At 2011-10-25 20:30:39,"Ben Skeggs"  wrote:
>On Tue, 2011-10-25 at 14:15 +0200, Francisco Jerez wrote:
>> Maarten Maathuis  writes:
>> 
>> > 2011/10/25 chris :
>> >> Can anyone give a suggestion, is wait-vblank fully implemented in
>> >> page_flip() for nouveau drm driver?
>> >>
>> 
>> It's intentionally not implemented.  The reason is that I wanted to
>> support non-vsync'ed vblank as well, and for vsync'ed blits we had to
>> think about a different mechanism for vblank synchronization anyway, so
>> I figured it didn't make that much sense to force vblank synchronization
>> directly from the pageflip ioctl.
>+1 I deliberately didn't flip 1 bit in the NV50/NVC0 page flipping code
>for this as well.  The interface IMO is flawed.  Though, that said, we
>really should look at doing something properly for this, a lot of people
>do want tear-free goodness.
>
>Ben.
>> 
>> >>
>> >> At 2011-10-24 14:30:55,chris  wrote:
>> >>
>> >> Dear,
>> >>
>> >> I use NVidia Geforce 7300GT graphics card in my PC, and Linux 3.1rc4 
>> >> kernel
>> >> code, git drm 2.4.36.
>> >>   When I run the vbltest program, it prints  "60HZ"  which indicated the
>> >> implementation of drmWaitVBlank() and drm_vblank_wait()  is correct.
>> >>   But when I run modetest with option " -v -s 12:1280x1024" , it prints 
>> >> high
>> >> fresh rate up to "150 HZ" .  I examing the code , and found that no 
>> >> waiting
>> >> vblank operation is processed in nouveau_crtc_ page_flip() function. The
>> >> screen  produced lots of garbage and blink very much.
>> 
>> That's fine if by "garbage" you just mean it's tearing like crazy.
>> 
>> >>[...]
>> >
>> > It seems to be, the actual page flipping is done by software method
>> > (see nv04_graph_mthd_page_flip). There is one thing i'm unsure about
>> > and that is that we wait for the rendering to be done to the current
>> > frontbuffer and not the current backbuffer (this is only done if the
>> > page flip channel is different than the rendering channel). Maybe
>> > someone else can comment on that.
>> 
>> There's no need to wait for the backbuffer rendering to end because the
>> pageflip is always pushed through the last channel that has queued
>> rendering to it, so, the "waiting" is actually done by the GPU. The
>> waiting to the current frontbuffer (which in most cases is going to be a
>> cross-channel barrier instead of actual CPU waiting) is necessary for
>> the (rare) case where you have several channels trying to render to the
>> same pageflipped drawable, to make sure that the flips are properly
>> synchronized with respect each other.
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>___
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panfrost: devfreq: Don't display error for EPROBE_DEFER

[Chris Morgan]

From: Chris Morgan 

Set a condition for the message of "Couldn't set OPP regulators" to not
display if the error code is EPROBE_DEFER. Note that I used an if
statement to capture the condition instead of the dev_err_probe
function because I didn't want to change the DRM_DEV_ERROR usage.

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 3644652f726f..194af7f607a6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -106,7 +106,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV) {
-   DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
+   if (ret != -EPROBE_DEFER)
+   DRM_DEV_ERROR(dev, "Couldn't set OPP 
regulators\n");
return ret;
}
}
-- 
2.25.1

TGL: : No video output on external monitor after unplug and re-plug the cable

[Chris Chiu]

We found another bug after the fix of
https://gitlab.freedesktop.org/drm/intel/-/issues/2538. The external
monitor is also connected via WD19's HDMI/DisplayPort just as #2538.
However, the display monitor can only be detected and show output at
the very first time we power on the WD19 dock. If we unplug the cable
and replug again, the monitor seems to be detected but there's no
video output.

When we power on the WD19 dock with cable connected to the monitor,
the drm kernel log shows as follows

 i915 :00:02.0: [drm:intel_get_hpd_pins.isra.0 [i915]] hotplug
event received, stat 0x0001, dig 0x008a, pins 0x0200, long
0x0200
 i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]] digital hpd on
[ENCODER:292:DDI D] - long
 i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]] Received HPD
interrupt on PIN 9 - cnt: 10
 i915 :00:02.0: [drm:intel_dp_hpd_pulse [i915]] got hpd irq on
[ENCODER:292:DDI D] - long
 i915 :00:02.0: [drm:i915_hotplug_work_func [i915]] running
encoder hotplug functions
 i915 :00:02.0: [drm:i915_hotplug_work_func [i915]] Connector DP-1
(pin 9) received hotplug event. (retry 0)
 i915 :00:02.0: [drm:intel_dp_detect [i915]] [CONNECTOR:293:DP-1]
 i915 :00:02.0: [drm:intel_power_well_enable [i915]] enabling TC cold off
 i915 :00:02.0: [drm:tgl_tc_cold_request [i915]] TC cold block succeeded
 i915 :00:02.0: [drm:__intel_tc_port_lock [i915]] Port D/TC#1: TC
port mode reset (tbt-alt -> dp-alt)
 i915 :00:02.0: [drm:intel_power_well_enable [i915]] enabling AUX D TC1
 i915 :00:02.0: [drm:drm_dp_dpcd_read [drm_kms_helper]] AUX D/port
D: 0xf AUX -> (ret=  8) 14 1e 40 55 02 00 00 00
 i915 :00:02.0: [drm:intel_dp_lttpr_init [i915]] LTTPR common
capabilities: 14 1e 40 55 02 00 00 00

Then I replug the cable, the intel_power_well_enable() in
intel_dp_aux_xfer() shows "enabling DC off" power domain instead of
enabling AUX D TC1. After that, the flooded i915 :00:02.0:
[drm:intel_dp_aux_xfer [i915]] AUX D/port D: timeout (status
0x7d4003ff) keeps show up and no video output.

I filed a bug on
https://gitlab.freedesktop.org/drm/intel/-/issues/3407 and also
uploaded the journal log  with kernel boot parameter
"drm.debug=0x10e".

Can anyone suggest what happens at the replug? What can we do to
identify the cause? Thanks

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

[PATCH] drm/panel: add rotation support for Elida KD35T133 panels

[Chris Morgan]

Update the panel to allow setting the rotation value in device tree.
Tested on an Odroid Go Advance, where the panel is by default rotated 270
degrees.

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c 
b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index bc36aa3c1123..d8534406d1ef 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -42,6 +42,7 @@ struct kd35t133 {
struct gpio_desc *reset_gpio;
struct regulator *vdd;
struct regulator *iovcc;
+   enum drm_panel_orientation orientation;
bool prepared;
 };
 
@@ -216,6 +217,7 @@ static int kd35t133_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = mode->width_mm;
connector->display_info.height_mm = mode->height_mm;
drm_mode_probed_add(connector, mode);
+   drm_connector_set_panel_orientation(connector, ctx->orientation);
 
return 1;
 }
@@ -258,6 +260,12 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
return ret;
}
 
+   ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
+   if (ret < 0) {
+   dev_err(dev, "%pOF: failed to get orientation %d\n", 
dev->of_node, ret);
+   return ret;
+   }
+
mipi_dsi_set_drvdata(dsi, ctx);
 
ctx->dev = dev;
-- 
2.25.1

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

[PATCH] drm/panel: correct typo in comments for Elida KD35T133

[Chris Morgan]

Update the comments to state this is a 3.5" display and not a 5.5" display.

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c 
b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index d8534406d1ef..bce84d2e6ada 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Elida kd35t133 5.5" MIPI-DSI panel driver
+ * Elida kd35t133 3.5" MIPI-DSI panel driver
  * Copyright (C) 2020 Theobroma Systems Design und Consulting GmbH
  *
  * based on
-- 
2.25.1

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

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:02:57)
> On Wed, Jan 13, 2021 at 10:08 PM Chris Wilson  
> wrote:
> > Quoting Daniel Vetter (2021-01-13 20:50:11)
> > > On Wed, Jan 13, 2021 at 4:43 PM Chris Wilson  
> > > wrote:
> > > >
> > > > Quoting Daniel Vetter (2021-01-13 14:06:04)
> > > > > We have too many people abusing the struct page they can get at but
> > > > > really shouldn't in importers. Aside from that the backing page might
> > > > > simply not exist (for dynamic p2p mappings) looking at it and using it
> > > > > e.g. for mmap can also wreak the page handling of the exporter
> > > > > completely. Importers really must go through the proper interface like
> > > > > dma_buf_mmap for everything.
> > > >
> > > > If the exporter doesn't want to expose the struct page, why are they
> > > > setting it in the exported sg_table?
> > >
> > > You need to store it somewhere, otherwise the dma-api doesn't work.
> > > Essentially this achieves clearing/resetting the struct page pointer,
> > > without additional allocations somewhere, or tons of driver changes
> > > (since presumably the driver does keep track of the struct page
> > > somewhere too).
> >
> > Only for mapping, and that's before the export -- if there's even a
> > struct page to begin with.
> >
> > > Also as long as we have random importers looking at struct page we
> > > can't just remove it, or crashes everywhere. So it has to be some
> > > debug option you can disable.
> >
> > Totally agreed that nothing generic can rely on pages being transported
> > via dma-buf, and memfd is there if you do want a suitable transport. The
> > one I don't know about is dma-buf heap, do both parties there consent to
> > transport pages via the dma-buf? i.e. do they have special cases for
> > import/export between heaps?
> 
> heaps shouldn't be any different wrt the interface exposed to
> importers. Adding John just in case I missed something.
> 
> I think the only problem we have is that the first import for ttm
> simply pulled out the struct page and ignored the sgtable otherwise,
> then that copypasted to places and we're still have some of that left.
> Although it's a lot better. So largely the problem is importers being
> a bit silly.
> 
> I also think I should change the defaulty y to default y if
> DMA_API_DEBUG or something like that, to make sure it's actually
> enabled often enough.

It felt overly draconian, but other than the open question of dma-buf
heaps (which I realise that we need some CI coverage for), I can't
think of a good reason to argue for hiding a struct page transport
within dma-buf.

The only other problem I see with the implementation is that there's
nothing that says that each dmabuf->ops->map_dma_buf() returns a new
sg_table, so we may end up undoing the xor. Or should each dma-buf
return a fresh dma-mapping for iommu isolation?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:30:32)
> On Thu, Jan 14, 2021 at 10:23 AM Chris Wilson  
> wrote:
> > The only other problem I see with the implementation is that there's
> > nothing that says that each dmabuf->ops->map_dma_buf() returns a new
> > sg_table, so we may end up undoing the xor. Or should each dma-buf
> > return a fresh dma-mapping for iommu isolation?
> 
> Maybe I screwed it up, but that's why I extracted the little helpers:
> We scramble when we get the sgtable from exporter, and unscramble
> before we pass it back. dma-buf.c does some caching and will hand back
> the same sgtable, but for that case we don't re-scramble.

The attachment is only mapped once, but there can be more than one
attachment, and the backend could return the same sg_table for each
mapping. Conceivably, it could return its own private sg_table where it
wants to maintain the struct page. Seems like just adding a sentence to
@map_dma_buf to clarify that each call should return a new sg_table will
suffice.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [BUG] on reboot: bisected to: drm/i915: Shut down displays gracefully on reboot

[Chris Wilson]

Quoting Steven Rostedt (2021-01-14 21:32:06)
> On reboot, one of my test boxes now triggers the following warning:

057fe3535eb3 ("drm/i915: Disable RPM wakeref assertions during driver shutdown")
is included with the drm-intel-fixes PR.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-14 09:47:40)
> On Thu, Jan 14, 2021 at 09:45:37AM +0000, Chris Wilson wrote:
> > Quoting Daniel Vetter (2021-01-14 09:30:32)
> > > On Thu, Jan 14, 2021 at 10:23 AM Chris Wilson  
> > > wrote:
> > > > The only other problem I see with the implementation is that there's
> > > > nothing that says that each dmabuf->ops->map_dma_buf() returns a new
> > > > sg_table, so we may end up undoing the xor. Or should each dma-buf
> > > > return a fresh dma-mapping for iommu isolation?
> > > 
> > > Maybe I screwed it up, but that's why I extracted the little helpers:
> > > We scramble when we get the sgtable from exporter, and unscramble
> > > before we pass it back. dma-buf.c does some caching and will hand back
> > > the same sgtable, but for that case we don't re-scramble.
> > 
> > The attachment is only mapped once, but there can be more than one
> > attachment, and the backend could return the same sg_table for each
> > mapping. Conceivably, it could return its own private sg_table where it
> > wants to maintain the struct page. Seems like just adding a sentence to
> > @map_dma_buf to clarify that each call should return a new sg_table will
> > suffice.
> 
> Ah yes good point, will augment (once CI stops being angry at me).

Fwiw, with a quick explanation of "don't do this" in the docs,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf: Add debug option

[Chris Wilson]

Quoting Daniel Vetter (2021-01-15 15:52:26)
> +static void mangle_sg_table(struct sg_table *sg_table)
> +{
> +#ifdef CONFIG_DMABUF_DEBUG
> +   int i;
> +   struct scatterlist *sg;
> +
> +   if (!sg_table)

if (!IS_ENABLED(CONFIG_DMABUF_DEBUG) || IS_ERR_OR_NULL(sg_table))
> +   return;

Although NULL is not meant to be returned.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early

[Chris Wilson]

Quoting Jinoh Kang (2021-01-15 16:23:31)
> If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
> returned only when the object is actually bound.
> 
> The xf86-video-intel userspace driver cannot differentiate this
> condition, and marks the GPU as wedged.

The idea was to call gem_set_domain on the object to validate the pages
after creation. I only did that for read-only... I did however make mesa
use set-domain for validation.

As a question how are you getting to call userptr on something that
wasn't passed by SHM ipc?

> This not only disables graphics
> acceleration but may also cripple other functions such as VT switch.

That should be a non-sequitur; certainly VT switch works without ever
using the GPU.

> Solve this by "prefaulting" user pages on GEM object creation, testing
> whether all pages are eligible for get_user_pages() in the process.
> On failure, return -EFAULT so that userspace can fallback to software
> blitting.

See https://patchwork.freedesktop.org/series/33449/ for adding PROBE |
POPULATE flags.

But we can just use set-domain.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early

[Chris Wilson]

Quoting Chris Wilson (2021-01-15 16:56:42)
> Quoting Jinoh Kang (2021-01-15 16:23:31)
> > If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
> > returned only when the object is actually bound.
> > 
> > The xf86-video-intel userspace driver cannot differentiate this
> > condition, and marks the GPU as wedged.
> 
> The idea was to call gem_set_domain on the object to validate the pages
> after creation. I only did that for read-only... I did however make mesa
> use set-domain for validation.

Hmm, I remember a reason why we wanted to be lazy in populating the
pages was that we would often be called to create a map that was never
used. So the additional gup + bind was measurable, and we would rather
just have a probe.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/6] drm/i915/gt: Remove references to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-18 13:14:17)
> Using struct drm_device.pdev is deprecated. Convert i915 to struct
> drm_device.dev. No functional changes.

This needs to be before or in the previous patch, as that patch removed
assignment of i915->drm.pdev.

Or the removal of the assignment moved to the end as a separate patch.
That makes more sense.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/selftest: Fix potential memory leak

[Chris Wilson]

Quoting Pan Bian (2021-01-22 01:56:40)
> Object out is not released on path that no VMA instance found. The root
> cause is jumping to an unexpected label on the error path.

Wouldn't the root cause be whatever caused the allocation to fail?

Language notwithstanding,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/gvt: fix uninitialized return in intel_gvt_update_reg_whitelist()

[Chris Wilson]

Quoting Dan Carpenter (2021-01-25 08:48:30)
> Smatch found an uninitialized variable bug in this code:
> 
> drivers/gpu/drm/i915/gvt/cmd_parser.c:3191 
> intel_gvt_update_reg_whitelist()
> error: uninitialized symbol 'ret'.
> 
> The first thing that Smatch complains about is that "ret" isn't set if
> we don't enter the "for_each_engine(engine, &dev_priv->gt, id) {" loop.
> Presumably we always have at least one engine so that's a false
> positive.
> 
> But it's definitely a bug to not set "ret" if i915_gem_object_pin_map()
> fails.

True.
 
> Let's fix the bug and silence the false positive.
> 
> Fixes: 493f30cd086e ("drm/i915/gvt: parse init context to update cmd 
> accessible reg whitelist")
> Signed-off-by: Dan Carpenter 
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Fix DRM_I915_WERROR dependencies

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-25 12:26:44)
> From: Arnd Bergmann 
> 
> CONFIG_DRM_I915_DEBUG now selects CONFIG_DRM_I915_WERROR, but fails
> to honor its dependencies:
> 
> WARNING: unmet direct dependencies detected for DRM_I915_WERROR
>   Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=m] && EXPERT [=y] && 
> !COMPILE_TEST [=y]
>   Selected by [m]:
>   - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && EXPERT [=y] && DRM_I915 [=m]

DRM_I915_DEBUG now depends on !COMPILE_TEST and EXPERT.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/gem: fix non-SMP build failure

[Chris Wilson]

Quoting Arnd Bergmann (2021-01-25 12:25:34)
> From: Arnd Bergmann 
> 
> The x86-specific wbinvd_on_all_cpus() function is exported
> through asm/smp.h, causing a build failure in the i915 driver
> when SMP is disabled:
> 
> drivers/gpu/drm/i915/i915_gem.c:1182:2: error: implicit declaration of 
> function 'wbinvd_on_all_cpus' [-Werror,-Wimplicit-function-declaration]

I thought the code was already in i915_gem_pm.c (which included smp.h);
it is now.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/i915/gt: use new tasklet API for execution list

[Chris Wilson]

Quoting Emil Renner Berthing (2021-01-26 15:01:55)
> This converts the driver to use the new tasklet API introduced in
> commit 12cc923f1ccc ("tasklet: Introduce new initialization API")
> 
> Signed-off-by: Emil Renner Berthing 
> 
> ---
> v2: Rebased on drm-intel-next

Ta. Saves me having to do the fixup.

Reviewed-by: Chris Wilson 

Will be applied to drm-intel-gt-next which is scheduled for inclusion in
5.13. It should apply against the 5.12 merge window if there's a tree
through which you want to migrate the tasklet API faster.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/5] drm/i915: Remove references to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-27 12:41:31)
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c 
> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 0188f877cab2..2a07a008de2e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -146,7 +146,6 @@ struct drm_i915_private *mock_gem_device(void)
> }
>  
> pci_set_drvdata(pdev, i915);
> -   i915->drm.pdev = pdev;

Strictly this removal is still too early. Though it's a mock device and
we shouldn't be touching the pci_dev that often, semantically those
accesses are not removed until later.

>  
> dev_pm_domain_set(&pdev->dev, &pm_domain);
> pm_runtime_enable(&pdev->dev);
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gtt.c 
> b/drivers/gpu/drm/i915/selftests/mock_gtt.c
> index 7270fc8ca801..5c7ae40bba63 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gtt.c
> @@ -74,7 +74,7 @@ struct i915_ppgtt *mock_ppgtt(struct drm_i915_private 
> *i915, const char *name)
> ppgtt->vm.i915 = i915;
> ppgtt->vm.total = round_down(U64_MAX, PAGE_SIZE);
> ppgtt->vm.file = ERR_PTR(-ENODEV);
> -   ppgtt->vm.dma = &i915->drm.pdev->dev;
> +   ppgtt->vm.dma = i915->drm.dev;

We can remove this shorthand later.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 4/5] drm/i915: Don't assign to struct drm_device.pdev

[Chris Wilson]

Quoting Thomas Zimmermann (2021-01-27 12:41:34)
> Using struct drm_device.pdev is deprecated. Don't assign it. Users
> should upcast from struct drm_device.dev.
> 
> Signed-off-by: Thomas Zimmermann 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 700aeb923fcd..954ad590089c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -787,7 +787,6 @@ i915_driver_create(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
> if (IS_ERR(i915))
> return i915;
>  
> -   i915->drm.pdev = pdev;
> pci_set_drvdata(pdev, i915);
>  
> /* Device parameters start as a copy of module parameters. */

Stick the mock
-   i915->drm.pdev = pdev;
in this patch, and I'm happy.

With that, the series is
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] linux-next: Tree for Jan 27 (drm/i915)

[Chris Wilson]

Quoting Randy Dunlap (2021-01-27 20:28:05)
> On 1/27/21 11:30 AM, Randy Dunlap wrote:
> > On 1/27/21 11:08 AM, Randy Dunlap wrote:
> >> On 1/27/21 6:44 AM, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Note: the patch file has failed to upload :-(
> >>>
> >>> Changes since 20210125:
> >>>
> >>
> >> on x86_64:
> >>
> >> ../drivers/gpu/drm/i915/i915_gem.c: In function ‘i915_gem_freeze_late’:
> >> ../drivers/gpu/drm/i915/i915_gem.c:1182:2: error: implicit declaration of 
> >> function ‘wbinvd_on_all_cpus’; did you mean ‘wrmsr_on_cpus’? 
> >> [-Werror=implicit-function-declaration]
> >>   wbinvd_on_all_cpus();
> >>
> > 
> > My apologies: this was in Andrew's mmotm 2021-01-25.
> > Sorry about that.
> 
> 
> and now I see that it also happens in today's linux-next.

The fix is in the tree that should be feeding into linux-next, so I
trust it will resolve itself shortly. Along with the WERROR depends
snafu.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Remove unreachable code

[Chris Wilson]

Quoting Vinicius Tinti (2021-01-29 18:15:19)
> By enabling -Wunreachable-code-aggressive on Clang the following code
> paths are unreachable.

That code exists as commentary and, especially for sdvo, library
functions that we may need in future.

The ivb-gt1 case => as we now set the gt level for ivb, should we not
enable the optimisation for ivb unaffected by the w/a? Just no one has
taken the time to see if it causes a regression.

For error state, the question remains whether we should revert to
uncompressed data if the compressed stream is larger than the original.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Remove unreachable code

[Chris Wilson]

Quoting Vinicius Tinti (2021-01-30 12:34:11)
> On Fri, Jan 29, 2021 at 08:55:54PM +0000, Chris Wilson wrote:
> > Quoting Vinicius Tinti (2021-01-29 18:15:19)
> > > By enabling -Wunreachable-code-aggressive on Clang the following code
> > > paths are unreachable.
> > 
> > That code exists as commentary and, especially for sdvo, library
> > functions that we may need in future.
> 
> I would argue that this code could be removed since it is in git history.
> It can be restored when needed.
> 
> This will make the code cleaner.

It doesn't change the control flow, so no complexity argument. It
removes documentation from the code, so I have the opposite opinion.

> > The ivb-gt1 case => as we now set the gt level for ivb, should we not
> > enable the optimisation for ivb unaffected by the w/a? Just no one has
> > taken the time to see if it causes a regression.
> 
> I don't know. I just found out that the code is unreachable.
> 
> > For error state, the question remains whether we should revert to
> > uncompressed data if the compressed stream is larger than the original.
> 
> I don't know too.
> 
> In this last two cases the code could be commented and the decisions
> and problems explained in the comment section.

They already are, that is the point.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] v5.11-rc5 BUG kmalloc-1k (Not tainted): Redzone overwritten

[Chris Wilson]

Quoting Jani Nikula (2021-01-28 13:23:48)
> 
> A number of our CI systems are hitting redzone overwritten errors after
> s2idle, with the errors introduced between v5.11-rc4 and v5.11-rc5. See
> snippet below, full logs for one affected machine at [1].
> 
> Known issue?

Fwiw, I think this should be fixed by

commit 08d60e5999540110576e7c1346d486220751b7f9
Author: John Ogness 
Date:   Sun Jan 24 21:33:28 2021 +0106

printk: fix string termination for record_print_text()

Commit f0e386ee0c0b ("printk: fix buffer overflow potential for
print_text()") added string termination in record_print_text().
However it used the wrong base pointer for adding the terminator.
This led to a 0-byte being written somewhere beyond the buffer.

Use the correct base pointer when adding the terminator.

Fixes: f0e386ee0c0b ("printk: fix buffer overflow potential for 
print_text()")
Reported-by: Sven Schnelle 
Signed-off-by: John Ogness 
Signed-off-by: Petr Mladek 
Link: 
https://lore.kernel.org/r/20210124202728.4718-1-john.ogn...@linutronix.de

din should be rolled forward, but there's yet another regression in rc6
breaking suspend on all machines.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 1/3] proc: Show GPU runtimes

[Chris Wilson]

Present an interface for system monitors to watch the GPU usage as a
whole and by individual applications. By consolidating the information
into a canonical location, we have a single interface that can track the
utilisation of all GPU devices and sub-devices. This is preferrable to
asking the system monitors to walk the sysfs, or other interfaces, of
each device and parse the custom information presented by each driver.

Opens:
- Should we try to name each channel so that it can be shown in UI?

In gnome-system-monitor, we would have a task list:
Process ... GPU0% GPU1%
and charts that would show the GPU% on/next the CPU overview.

Then we could have a futher expansion of a GPU% into per-channel
utilisation. That would be useful to check to see what is saturating a
particular channel, e.g. find the video decoder bottleneck.

Signed-off-by: Chris Wilson 
---
 fs/proc/Makefile |  1 +
 fs/proc/base.c   |  2 +
 fs/proc/gpu.c| 83 
 fs/proc/internal.h   |  6 +++
 include/linux/proc_gpu.h | 33 
 5 files changed, 125 insertions(+)
 create mode 100644 fs/proc/gpu.c
 create mode 100644 include/linux/proc_gpu.h

diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index bd08616ed8ba..bdc42b592e3e 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -16,6 +16,7 @@ proc-y+= cmdline.o
 proc-y += consoles.o
 proc-y += cpuinfo.o
 proc-y += devices.o
+proc-y += gpu.o
 proc-y += interrupts.o
 proc-y += loadavg.o
 proc-y += meminfo.o
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b3422cda2a91..062298f5f6c8 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3266,6 +3266,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_SECCOMP_CACHE_DEBUG
ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
 #endif
+   ONE("gpu", S_IRUGO, proc_pid_gpu),
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
@@ -3598,6 +3599,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_SECCOMP_CACHE_DEBUG
ONE("seccomp_cache", S_IRUSR, proc_pid_seccomp_cache),
 #endif
+   ONE("gpu", S_IRUGO, proc_pid_gpu),
 };
 
 static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
diff --git a/fs/proc/gpu.c b/fs/proc/gpu.c
new file mode 100644
index ..7264bf1f2f7b
--- /dev/null
+++ b/fs/proc/gpu.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "internal.h"
+
+static LIST_HEAD(gpu);
+static DEFINE_SPINLOCK(lock);
+
+void proc_gpu_register(struct proc_gpu *pg)
+{
+   spin_lock(&lock);
+   list_add_tail(&pg->link, &gpu);
+   spin_unlock(&lock);
+}
+EXPORT_SYMBOL_GPL(proc_gpu_register);
+
+void proc_gpu_unregister(struct proc_gpu *pg)
+{
+   spin_lock(&lock);
+   list_del(&pg->link);
+   spin_unlock(&lock);
+}
+EXPORT_SYMBOL_GPL(proc_gpu_unregister);
+
+static void print_runtime(struct seq_file *m, const struct proc_gpu_runtime 
*rt)
+{
+   int i;
+
+   seq_printf(m, "%llu", rt->device);
+
+   for (i = 0; i < rt->nchannel; i++)
+   seq_printf(m, " %llu", rt->channel[i]);
+
+   seq_printf(m, " %s\n", rt->name);
+}
+
+int proc_pid_gpu(struct seq_file *m, struct pid_namespace *ns,
+struct pid *pid, struct task_struct *task)
+{
+   struct proc_gpu *p, *pn, mark = {};
+   struct proc_gpu_runtime rt;
+
+   spin_lock(&lock);
+   list_for_each_entry_safe(p, pn, &gpu, link) {
+   if (!p->fn)
+   continue;
+
+   rt.name[0] = '\0';
+   p->fn(p, pid, &rt);
+   if (!rt.name[0])
+   continue;
+
+   list_add(&mark.link, &p->link);
+   spin_unlock(&lock);
+
+   print_runtime(m, &rt);
+
+   spin_lock(&lock);
+   list_safe_reset_next(&mark, pn, link);
+   list_del(&mark.link);
+   }
+   spin_unlock(&lock);
+
+   return 0;
+}
+
+static int proc_gpu_show(struct seq_file *m, void *v)
+{
+   return proc_pid_gpu(m, NULL, NULL, NULL);
+}
+
+static int __init proc_gpu_init(void)
+{
+   proc_create_single("gpu", 0, NULL, proc_gpu_show);
+   return 0;
+}
+fs_initcall(proc_gpu_init);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index f60b379dcdc7..08bf45bec975 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -221,6 +221,12 @@ void set_proc_pid_nlink(void);
 extern struct inode *proc_get_inode(struct super_block *, struct 
proc_dir_entry *);
 extern void proc_entry_rundown(struct proc_dir_entry *);
 
+/*
+ * proc_gpu.c
+ */
+int proc_pid_gpu(struct seq_file *m, struct pid_namespace *

[RFC 2/3] drm/i915: Look up clients by pid

[Chris Wilson]

Use the pid to find associated clients, and report their runtime. This
will be used to provide the information via procfs.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_drm_client.c | 70 +++---
 drivers/gpu/drm/i915/i915_drm_client.h | 12 +++--
 2 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drm_client.c 
b/drivers/gpu/drm/i915/i915_drm_client.c
index 1f8b08a413d4..52d9ae97ba25 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -26,6 +26,9 @@ void i915_drm_clients_init(struct i915_drm_clients *clients,
 
clients->next_id = 0;
xa_init_flags(&clients->xarray, XA_FLAGS_ALLOC);
+
+   hash_init(clients->pids);
+   spin_lock_init(&clients->pid_lock);
 }
 
 static ssize_t
@@ -95,6 +98,50 @@ show_busy(struct device *kdev, struct device_attribute 
*attr, char *buf)
return sysfs_emit(buf, "%llu\n", total);
 }
 
+u64 i915_drm_clients_get_runtime(struct i915_drm_clients *clients,
+struct pid *pid,
+u64 *rt)
+{
+   struct i915_drm_client_name *name;
+   u64 total = 0;
+   u64 t;
+
+   memset64(rt, 0, MAX_ENGINE_CLASS + 1);
+
+   rcu_read_lock();
+   hash_for_each_possible_rcu(clients->pids, name, node, pid_nr(pid)) {
+   struct i915_drm_client *client = name->client;
+   struct list_head *list = &client->ctx_list;
+   struct i915_gem_context *ctx;
+   int i;
+
+   if (name->pid != pid)
+   continue;
+
+   for (i = 0; i < ARRAY_SIZE(client->past_runtime); i++) {
+   t = atomic64_read(&client->past_runtime[i]);
+   rt[i] += t;
+   total += t;
+   }
+
+   list_for_each_entry_rcu(ctx, list, client_link) {
+   struct i915_gem_engines_iter it;
+   struct intel_context *ce;
+
+   for_each_gem_engine(ce,
+   rcu_dereference(ctx->engines),
+   it) {
+   t = intel_context_get_total_runtime_ns(ce);
+   rt[ce->engine->class] += t;
+   total += t;
+   }
+   }
+   }
+   rcu_read_unlock();
+
+   return total;
+}
+
 static const char * const uabi_class_names[] = {
[I915_ENGINE_CLASS_RENDER] = "0",
[I915_ENGINE_CLASS_COPY] = "1",
@@ -242,7 +289,10 @@ __i915_drm_client_register(struct i915_drm_client *client,
if (!name)
return -ENOMEM;
 
+   spin_lock(&clients->pid_lock);
+   hash_add_rcu(clients->pids, &name->node, pid_nr(name->pid));
RCU_INIT_POINTER(client->name, name);
+   spin_unlock(&clients->pid_lock);
 
if (!clients->root)
return 0; /* intel_fbdev_init registers a client before sysfs */
@@ -254,20 +304,25 @@ __i915_drm_client_register(struct i915_drm_client *client,
return 0;
 
 err_sysfs:
+   spin_lock(&clients->pid_lock);
RCU_INIT_POINTER(client->name, NULL);
+   hash_del_rcu(&name->node);
+   spin_unlock(&clients->pid_lock);
call_rcu(&name->rcu, free_name);
return ret;
 }
 
 static void __i915_drm_client_unregister(struct i915_drm_client *client)
 {
+   struct i915_drm_clients *clients = client->clients;
struct i915_drm_client_name *name;
 
__client_unregister_sysfs(client);
 
-   mutex_lock(&client->update_lock);
+   spin_lock(&clients->pid_lock);
name = rcu_replace_pointer(client->name, NULL, true);
-   mutex_unlock(&client->update_lock);
+   hash_del_rcu(&name->node);
+   spin_unlock(&clients->pid_lock);
 
call_rcu(&name->rcu, free_name);
 }
@@ -294,7 +349,6 @@ i915_drm_client_add(struct i915_drm_clients *clients, 
struct task_struct *task)
return ERR_PTR(-ENOMEM);
 
kref_init(&client->kref);
-   mutex_init(&client->update_lock);
spin_lock_init(&client->ctx_lock);
INIT_LIST_HEAD(&client->ctx_list);
 
@@ -339,16 +393,20 @@ int
 i915_drm_client_update(struct i915_drm_client *client,
   struct task_struct *task)
 {
+   struct i915_drm_clients *clients = client->clients;
struct i915_drm_client_name *name;
 
name = get_name(client, task);
if (!name)
return -ENOMEM;
 
-   mutex_lock(&client->update_lock);
-   if (name->pid != rcu_dereference_protected(client->name, true)->pid)
+   spin_lo

[RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Register with /proc/gpu to provide the client runtimes for generic
top-like overview, e.g. gnome-system-monitor can use this information to
show the per-process multi-GPU usage.

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/Makefile|  1 +
 drivers/gpu/drm/i915/gt/intel_gt.c   |  5 ++
 drivers/gpu/drm/i915/gt/intel_gt_proc.c  | 66 
 drivers/gpu/drm/i915/gt/intel_gt_proc.h  | 14 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h |  3 ++
 5 files changed, 89 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_proc.c
 create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_proc.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ce01634d4ea7..16171f65f5d1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -104,6 +104,7 @@ gt-y += \
gt/intel_gt_irq.o \
gt/intel_gt_pm.o \
gt/intel_gt_pm_irq.o \
+   gt/intel_gt_proc.o \
gt/intel_gt_requests.o \
gt/intel_gtt.o \
gt/intel_llc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index ca76f93bc03d..72199c13330d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -12,6 +12,7 @@
 #include "intel_gt_buffer_pool.h"
 #include "intel_gt_clock_utils.h"
 #include "intel_gt_pm.h"
+#include "intel_gt_proc.h"
 #include "intel_gt_requests.h"
 #include "intel_mocs.h"
 #include "intel_rc6.h"
@@ -373,6 +374,8 @@ void intel_gt_driver_register(struct intel_gt *gt)
intel_rps_driver_register(>->rps);
 
debugfs_gt_register(gt);
+
+   intel_gt_driver_register__proc(gt);
 }
 
 static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
@@ -656,6 +659,8 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
 {
intel_wakeref_t wakeref;
 
+   intel_gt_driver_unregister__proc(gt);
+
intel_rps_driver_unregister(>->rps);
 
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_proc.c 
b/drivers/gpu/drm/i915/gt/intel_gt_proc.c
new file mode 100644
index ..42db22326c7c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_proc.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include 
+
+#include "i915_drm_client.h"
+#include "i915_drv.h"
+#include "intel_gt.h"
+#include "intel_gt_pm.h"
+#include "intel_gt_proc.h"
+
+static void proc_runtime_pid(struct intel_gt *gt,
+struct pid *pid,
+struct proc_gpu_runtime *rt)
+{
+   struct i915_drm_clients *clients = >->i915->clients;
+
+   BUILD_BUG_ON(MAX_ENGINE_CLASS >= ARRAY_SIZE(rt->channel));
+
+   rt->device = i915_drm_clients_get_runtime(clients, pid, rt->channel);
+   rt->nchannel = MAX_ENGINE_CLASS + 1;
+}
+
+static void proc_runtime_device(struct intel_gt *gt,
+   struct pid *pid,
+   struct proc_gpu_runtime *rt)
+{
+   struct intel_engine_cs *engine;
+   enum intel_engine_id id;
+   ktime_t dummy;
+
+   rt->nchannel = 0;
+   for_each_engine(engine, gt, id) {
+   rt->channel[rt->nchannel++] =
+   intel_engine_get_busy_time(engine, &dummy);
+   if (rt->nchannel == ARRAY_SIZE(rt->channel))
+   break;
+   }
+   rt->device = intel_gt_get_awake_time(gt);
+}
+
+static void proc_runtime(struct proc_gpu *pg,
+struct pid *pid,
+struct proc_gpu_runtime *rt)
+{
+   struct intel_gt *gt = container_of(pg, typeof(*gt), proc);
+
+   strscpy(rt->name, dev_name(gt->i915->drm.dev), sizeof(rt->name));
+   if (pid)
+   proc_runtime_pid(gt, pid, rt);
+   else
+   proc_runtime_device(gt, pid, rt);
+}
+
+void intel_gt_driver_register__proc(struct intel_gt *gt)
+{
+   gt->proc.fn = proc_runtime;
+   proc_gpu_register(>->proc);
+}
+
+void intel_gt_driver_unregister__proc(struct intel_gt *gt)
+{
+   proc_gpu_unregister(>->proc);
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_proc.h 
b/drivers/gpu/drm/i915/gt/intel_gt_proc.h
new file mode 100644
index ..7a9bff0fb020
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gt_proc.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#ifndef INTEL_GT_PROC_H
+#define INTEL_GT_PROC_H
+
+struct intel_gt;
+
+void intel_gt_driver_register__proc(struct intel_gt *gt);
+void intel_gt_driver_unregister__proc(struct intel_gt *gt);
+
+#endif /* INTEL_GT_PROC_H */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 
b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index 626af37c7790..3fc6d974176

[PATCH] drm/i915: Autoselect CONFIG_CHECKPOINT_RESTORE for SYS_kcmp

[Chris Wilson]

gallium (iris) depends on os_same_file_description() to disambiguate
screens and so avoid importing the same screen fd twice as two distinct
entities (that share all the kernel resources, so actions on screen
affect the other and would cause random faiure). As they depend on it,
so must we. os_same_file_description() uses SYS_kcmp to check the file
tables for the equivalent struct file, but SYS_kcmp is hidden behind
CONFIG_CHECKPOINT_RESTORE. As this is not default, we must select it for
ourselves to ensure that our userspace is fully supported.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Jani Nikula 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/i915/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 1e1cb245fca7..470a5214bd33 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -21,6 +21,7 @@ config DRM_I915
select ACPI_VIDEO if ACPI
select ACPI_BUTTON if ACPI
select SYNC_FILE
+   select CHECKPOINT_RESTORE # gallium depends on SYS_kcmp
select IOSF_MBI
select CRC32
select SND_HDA_I915 if SND_HDA_CORE
-- 
2.20.1

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

[PATCH] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
---
 init/Kconfig  | 11 +++
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..f62fca13ac5b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   default y
+   help
+ Enable the file descriptor comparison system call. It provides
+ user-space with the ability to compare two fd to see if they
+ point to the same file, and check other attributes.
+
+ If unsure, say Y.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret, 0);
if (ret != 0 && errno == ENOSYS)
-   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_CHECKPOINT_RESTORE?)");
+   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_KCMP?)");
 }
 
 TEST(mode_strict_support)
-- 
2.20.1

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

[PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Note that some distributions such as Ubuntu are already enabling
CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
Acked-by: Daniel Vetter  # DRM depends on SYS_kcmp

---
v2:
  - Default n.
  - Borrrow help message from man kcmp.
  - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
---
 fs/eventpoll.c|  4 ++--
 include/linux/eventpoll.h |  2 +-
 init/Kconfig  | 12 
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 5 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..3196474cbe24 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, struct 
file *file, int fd)
return epir;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned long 
toff)
 {
struct rb_node *rbp;
@@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
int tfd,
 
return file_raw;
 }
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_KCMP */
 
 /**
  * Adds a new entry to the tail of the list in a lockless way, i.e.
diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
index 0350393465d4..593322c946e6 100644
--- a/include/linux/eventpoll.h
+++ b/include/linux/eventpoll.h
@@ -18,7 +18,7 @@ struct file;
 
 #ifdef CONFIG_EPOLL
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned long 
toff);
 #endif
 
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..1b75141bc18b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,17 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   default n
+   help
+ Enable the kernel resource comparison system call. It provides
+ user-space with the ability to compare two processes to see if they
+ share a common resource, such as a file descriptor or even virtual
+ memory space.
+
+ If unsure, say N.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret, 0);
if (ret != 0 && errno == ENOSYS)
-   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_CHECKPOINT_RESTORE?)");
+   SKIP(return, "Kernel does not support kcmp() (missing 
CONFIG_KCMP?)");
 }
 
 TEST(mode_strict_support)
-- 
2.20.1

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

Re: [PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

The subject should of course be changed, as it is no longer being
enabled by default.

Something like

kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

Quoting Chris Wilson (2021-02-05 21:06:10)
> Userspace has discovered the functionality offered by SYS_kcmp and has
> started to depend upon it. In particular, Mesa uses SYS_kcmp for
> os_same_file_description() in order to identify when two fd (e.g. device
> or dmabuf) point to the same struct file. Since they depend on it for
> core functionality, lift SYS_kcmp out of the non-default
> CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.
> 
> Note that some distributions such as Ubuntu are already enabling
> CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.
> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
> Signed-off-by: Chris Wilson 
> Cc: Kees Cook 
> Cc: Andy Lutomirski 
> Cc: Will Drewry 
> Cc: Andrew Morton 
> Cc: Dave Airlie 
> Cc: Daniel Vetter 
> Cc: Lucas Stach 
> Acked-by: Daniel Vetter  # DRM depends on SYS_kcmp
> 
> ---
> v2:
>   - Default n.
>   - Borrrow help message from man kcmp.
>   - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
> ---
>  fs/eventpoll.c|  4 ++--
>  include/linux/eventpoll.h |  2 +-
>  init/Kconfig  | 12 
>  kernel/Makefile   |  2 +-
>  tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
>  5 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index a829af074eb5..3196474cbe24 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, 
> struct file *file, int fd)
> return epir;
>  }
>  
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> +#ifdef CONFIG_KCMP
>  static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned 
> long toff)
>  {
> struct rb_node *rbp;
> @@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
> int tfd,
>  
> return file_raw;
>  }
> -#endif /* CONFIG_CHECKPOINT_RESTORE */
> +#endif /* CONFIG_KCMP */
>  
>  /**
>   * Adds a new entry to the tail of the list in a lockless way, i.e.
> diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
> index 0350393465d4..593322c946e6 100644
> --- a/include/linux/eventpoll.h
> +++ b/include/linux/eventpoll.h
> @@ -18,7 +18,7 @@ struct file;
>  
>  #ifdef CONFIG_EPOLL
>  
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> +#ifdef CONFIG_KCMP
>  struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned 
> long toff);
>  #endif
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index b77c60f8b963..1b75141bc18b 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1194,6 +1194,7 @@ endif # NAMESPACES
>  config CHECKPOINT_RESTORE
> bool "Checkpoint/restore support"
> select PROC_CHILDREN
> +   select KCMP
> default n
> help
>   Enables additional kernel features in a sake of checkpoint/restore.
> @@ -1737,6 +1738,17 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
>  config ARCH_HAS_MEMBARRIER_SYNC_CORE
> bool
>  
> +config KCMP
> +   bool "Enable kcmp() system call" if EXPERT
> +   default n
> +   help
> + Enable the kernel resource comparison system call. It provides
> + user-space with the ability to compare two processes to see if they
> + share a common resource, such as a file descriptor or even virtual
> + memory space.
> +
> + If unsure, say N.
> +
>  config RSEQ
> bool "Enable rseq() system call" if EXPERT
> default y
> diff --git a/kernel/Makefile b/kernel/Makefile
> index aa7368c7eabf..320f1f3941b7 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -51,7 +51,7 @@ obj-y += livepatch/
>  obj-y += dma/
>  obj-y += entry/
>  
> -obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
> +obj-$(CONFIG_KCMP) += kcmp.o
>  obj-$(CONFIG_FREEZER) += freezer.o
>  obj-$(CONFIG_PROFILING) += profile.o
>  obj-$(CONFIG_STACKTRACE) += stacktrace.o
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
> b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 26c72f2b61b1..1b6c7d33c4ff 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -315,7 +315,7 @@ TEST(kcmp)
> ret = __filecmp(getpid(), getpid(), 1, 1);
> EXPECT_EQ(ret, 0);
> if (ret != 0 && errno == ENOSYS)
> -   SKIP(return, "Kernel does not support kcmp() (missing 
> CONFIG_CHECKPOINT_RESTORE?)");
> +   SKIP(return, "Kernel does not support kcmp() (missing 
> CONFIG_KCMP?)");
>  }
>  
>  TEST(mode_strict_support)
> -- 
> 2.20.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] kernel: Expose SYS_kcmp by default

[Chris Wilson]

Quoting Kees Cook (2021-02-05 21:20:33)
> On Fri, Feb 05, 2021 at 09:16:01PM +0000, Chris Wilson wrote:
> > The subject should of course be changed, as it is no longer being
> > enabled by default.
> 
> "default n" is redundant.

I thought being explicit would be preferred. There are a few other
default n, so at least it's not the odd-one-out!

> I thought Daniel said CONFIG_DRM needed to
> "select" it too, though?

Yes. We will need to select it for any DRM driver so that the Vulkan/GL
stacks can rely on having SYS_kcmp. That deserves to be handled and
explain within drm/Kconfig, and as they are already shipping with calls
to SYS_kcmp we may have to ask for a stable backport.

> Otherwise, yeah, this looks good. Was the
> export due to the 0-day bot failure reports?

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

[PATCH v3] kcmp: Support selection of SYS_kcmp without CHECKPOINT_RESTORE

[Chris Wilson]

Userspace has discovered the functionality offered by SYS_kcmp and has
started to depend upon it. In particular, Mesa uses SYS_kcmp for
os_same_file_description() in order to identify when two fd (e.g. device
or dmabuf) point to the same struct file. Since they depend on it for
core functionality, lift SYS_kcmp out of the non-default
CONFIG_CHECKPOINT_RESTORE into the selectable syscall category.

Rasmus Villemoes also pointed out that systemd uses SYS_kcmp to
deduplicate the per-service file descriptor store.

Note that some distributions such as Ubuntu are already enabling
CHECKPOINT_RESTORE in their configs and so, by extension, SYS_kcmp.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/3046
Signed-off-by: Chris Wilson 
Cc: Kees Cook 
Cc: Andy Lutomirski 
Cc: Will Drewry 
Cc: Andrew Morton 
Cc: Dave Airlie 
Cc: Daniel Vetter 
Cc: Lucas Stach 
Cc: Rasmus Villemoes 
Cc: Cyrill Gorcunov 
Cc: sta...@vger.kernel.org
Acked-by: Daniel Vetter  # DRM depends on kcmp
Acked-by: Rasmus Villemoes  # systemd uses kcmp

---
v2:
  - Default n.
  - Borrrow help message from man kcmp.
  - Export get_epoll_tfile_raw_ptr() for CONFIG_KCMP
v3:
  - Select KCMP for CONFIG_DRM
---
 drivers/gpu/drm/Kconfig   |  3 +++
 fs/eventpoll.c|  4 ++--
 include/linux/eventpoll.h |  2 +-
 init/Kconfig  | 11 +++
 kernel/Makefile   |  2 +-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  2 +-
 6 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 0973f408d75f..af6c6d214d91 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -15,6 +15,9 @@ menuconfig DRM
select I2C_ALGOBIT
select DMA_SHARED_BUFFER
select SYNC_FILE
+# gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
+# device and dmabuf fd. Let's make sure that is available for our userspace.
+   select KCMP
help
  Kernel-level support for the Direct Rendering Infrastructure (DRI)
  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a829af074eb5..3196474cbe24 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -979,7 +979,7 @@ static struct epitem *ep_find(struct eventpoll *ep, struct 
file *file, int fd)
return epir;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 static struct epitem *ep_find_tfd(struct eventpoll *ep, int tfd, unsigned long 
toff)
 {
struct rb_node *rbp;
@@ -1021,7 +1021,7 @@ struct file *get_epoll_tfile_raw_ptr(struct file *file, 
int tfd,
 
return file_raw;
 }
-#endif /* CONFIG_CHECKPOINT_RESTORE */
+#endif /* CONFIG_KCMP */
 
 /**
  * Adds a new entry to the tail of the list in a lockless way, i.e.
diff --git a/include/linux/eventpoll.h b/include/linux/eventpoll.h
index 0350393465d4..593322c946e6 100644
--- a/include/linux/eventpoll.h
+++ b/include/linux/eventpoll.h
@@ -18,7 +18,7 @@ struct file;
 
 #ifdef CONFIG_EPOLL
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
+#ifdef CONFIG_KCMP
 struct file *get_epoll_tfile_raw_ptr(struct file *file, int tfd, unsigned long 
toff);
 #endif
 
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..9cc7436b2f73 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1194,6 +1194,7 @@ endif # NAMESPACES
 config CHECKPOINT_RESTORE
bool "Checkpoint/restore support"
select PROC_CHILDREN
+   select KCMP
default n
help
  Enables additional kernel features in a sake of checkpoint/restore.
@@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
bool
 
+config KCMP
+   bool "Enable kcmp() system call" if EXPERT
+   help
+ Enable the kernel resource comparison system call. It provides
+ user-space with the ability to compare two processes to see if they
+ share a common resource, such as a file descriptor or even virtual
+ memory space.
+
+ If unsure, say N.
+
 config RSEQ
bool "Enable rseq() system call" if EXPERT
default y
diff --git a/kernel/Makefile b/kernel/Makefile
index aa7368c7eabf..320f1f3941b7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -51,7 +51,7 @@ obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
 
-obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
+obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c 
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 26c72f2b61b1..1b6c7d33c4ff 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -315,7 +315,7 @@ TEST(kcmp)
ret = __filecmp(getpid(), getpid(), 1, 1);
EXPECT_EQ(ret

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Anand Moon (2021-02-10 07:59:29)
> Add check for object size to return appropriate error -E2BIG or -EINVAL
> to avoid WARM_ON and sucessfull return for some testcase.

No. You miss the point of having those warnings. We need to inspect the
code to remove the last remaining "int pagenum", and then we can remove
the GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for
users, only for us to motivate us into finally fixing the code.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Quoting Emil Velikov (2021-02-12 14:57:56)
> Hi Chris,
> 
> On Thu, 4 Feb 2021 at 12:11, Chris Wilson  wrote:
> >
> > Register with /proc/gpu to provide the client runtimes for generic
> > top-like overview, e.g. gnome-system-monitor can use this information to
> > show the per-process multi-GPU usage.
> >
> Exposing this information to userspace sounds great IMHO and like the
> proposed "channels" for the device engines.
> If it were me, I would have the channel names a) exposed to userspace
> and b) be a "fixed set".

- Total
- Graphics
- Compute
- Unified
- Video
- Copy
- Display
- Other

Enough versatility for the foreseeable future?
But plan for extension.

The other aspect then is the capacity of each channel. We can keep it
simple as the union/average (whichever the driver has to hand) runtime in
nanoseconds over all IP blocks within a channel.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/panel: add rotation support for Elida KD35T133 panels

[Chris Morgan]

Update the panel to allow setting the rotation value in device tree.

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c 
b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index bc36aa3c1123..d8534406d1ef 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -42,6 +42,7 @@ struct kd35t133 {
struct gpio_desc *reset_gpio;
struct regulator *vdd;
struct regulator *iovcc;
+   enum drm_panel_orientation orientation;
bool prepared;
 };
 
@@ -216,6 +217,7 @@ static int kd35t133_get_modes(struct drm_panel *panel,
connector->display_info.width_mm = mode->width_mm;
connector->display_info.height_mm = mode->height_mm;
drm_mode_probed_add(connector, mode);
+   drm_connector_set_panel_orientation(connector, ctx->orientation);
 
return 1;
 }
@@ -258,6 +260,12 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
return ret;
}
 
+   ret = of_drm_get_panel_orientation(dev->of_node, &ctx->orientation);
+   if (ret < 0) {
+   dev_err(dev, "%pOF: failed to get orientation %d\n", 
dev->of_node, ret);
+   return ret;
+   }
+
mipi_dsi_set_drvdata(dsi, ctx);
 
ctx->dev = dev;
-- 
2.25.1

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

[PATCH 2/2] drm/panel: correct typo in comments for Elida KD35T133 panel driver

[Chris Morgan]

Update the comments to state this is a 3.5" display and not a 5.5" display

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c 
b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index d8534406d1ef..bce84d2e6ada 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Elida kd35t133 5.5" MIPI-DSI panel driver
+ * Elida kd35t133 3.5" MIPI-DSI panel driver
  * Copyright (C) 2020 Theobroma Systems Design und Consulting GmbH
  *
  * based on
-- 
2.25.1

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

Re: [RFC 3/3] drm/i915/gt: Export device and per-process runtimes via procfs

[Chris Wilson]

Quoting Emil Velikov (2021-02-12 15:45:04)
> On Fri, 12 Feb 2021 at 15:16, Chris Wilson  wrote:
> >
> > Quoting Emil Velikov (2021-02-12 14:57:56)
> > > Hi Chris,
> > >
> > > On Thu, 4 Feb 2021 at 12:11, Chris Wilson  
> > > wrote:
> > > >
> > > > Register with /proc/gpu to provide the client runtimes for generic
> > > > top-like overview, e.g. gnome-system-monitor can use this information to
> > > > show the per-process multi-GPU usage.
> > > >
> > > Exposing this information to userspace sounds great IMHO and like the
> > > proposed "channels" for the device engines.
> > > If it were me, I would have the channel names a) exposed to userspace
> > > and b) be a "fixed set".
> >
> > - Total
> > - Graphics
> > - Compute
> > - Unified
> > - Video
> > - Copy
> > - Display
> > - Other
> >
> > Enough versatility for the foreseeable future?
> > But plan for extension.
> >
> With a bit of documentation about "unified" (is it a metric also
> counted towards any of the rest) it would be perfect.

With unified I was trying to find a place to things that are neither
wholly graphics nor compute, as some may prefer not to categorise
themselves as one or the other. Also whether or not some cores are more
compute than others (so should there be an AI/RT/ALU?)

> For future extension one might consider splitting video into
> encoder/decoder/post-processing.

Ok, I wasn't sure how commonly those functions were split on different
HW.

> > The other aspect then is the capacity of each channel. We can keep it
> > simple as the union/average (whichever the driver has to hand) runtime in
> > nanoseconds over all IP blocks within a channel.
> 
> Not sure what you mean with capacity. Are you referring to having
> multiple instances of the same engine (say 3 separate copy engines)?
> Personally I'm inclined to keep these separate entries, since some
> hardware can have multiple ones.
> 
> For example - before the latest changes nouveau had 8 copy engines,
> 3+3 video 'generic' video (enc,dec)oder engines, amongst others.

Yes, most HW have multiple engines within a family. Trying to keep it
simple, I thought presenting just one runtime metric for the whole
channel. Especially for the single-line per device format I had picked :)

If we switch to a more extensible format,

-'$device0' : 
-$channel0 : {
Total : $total # avg/union over all engines
Engines : [ $0, $1, ... ]
}
...

-'$device1' : 
...

Using the same fixed channel names, and dev_name(), pesky concerns such
as keeping it as a simple scanf can be forgotten.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
> Hi Chris,
> 
> -Original Message-
> From: dri-devel  On Behalf Of Chris 
> Wilson
> Sent: Wednesday, February 10, 2021 4:15 PM
> To: Ram Moon, AnandX ; Jani Nikula 
> ; Auld, Matthew ; 
> Surendrakumar Upadhyay, TejaskumarX 
> ; Ursulin, Tvrtko 
> ; dri-devel@lists.freedesktop.org; 
> intel-...@lists.freedesktop.org
> Cc: Ram Moon, AnandX 
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size 
> for corner cases
> 
> Quoting Anand Moon (2021-02-10 07:59:29)
> > Add check for object size to return appropriate error -E2BIG or 
> > -EINVAL to avoid WARM_ON and successful return for some testcase.
> 
> No. You miss the point of having those warnings. We need to inspect the code 
> to remove the last remaining "int pagenum", and then we can remove the 
> GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, 
> only for us to motivate us into finally fixing the code.
> -Chris
> 
> Yes, I got your point these check are meant to catch up integer overflow.
> 
> I have check with the IGT testcase case  _gem_create_ and _gem_userptr_blits_ 
>  
> which fails pass size *-1ull << 32*  left shift and *0~* which leads to 
> integer overflow 
> ie  _negative_ size passed to create  i915_gem_create via ioctl  this leads 
> to GM_WARN_ON. 
> 
> Can we drop these testcase so that we don't break the kernel ?

The kernel rejects the ioctl with the expected errno. We leave a warning
purely for the benefit of CI, only when compiled for debugging and not by
default, so that we have a persistent reminder to do the code review.
What's broken?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size for corner cases

[Chris Wilson]

Quoting Ram Moon, AnandX (2021-02-16 12:05:23)
> Hi Chris,
> 
> -Original Message-
> From: dri-devel  On Behalf Of Chris 
> Wilson
> Sent: Monday, February 15, 2021 6:10 PM
> To: Auld, Matthew ; Ram Moon, AnandX 
> ; Surendrakumar Upadhyay, TejaskumarX 
> ; Ursulin, Tvrtko 
> ; Jani Nikula ; 
> dri-devel@lists.freedesktop.org; intel-...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object size 
> for corner cases
> 
> Quoting Ram Moon, AnandX (2021-02-15 12:29:17)
> > Hi Chris,
> > 
> > -----Original Message-
> > From: dri-devel  On Behalf Of 
> > Chris Wilson
> > Sent: Wednesday, February 10, 2021 4:15 PM
> > To: Ram Moon, AnandX ; Jani Nikula 
> > ; Auld, Matthew ; 
> > Surendrakumar Upadhyay, TejaskumarX 
> > ; Ursulin, Tvrtko 
> > ; dri-devel@lists.freedesktop.org; 
> > intel-...@lists.freedesktop.org
> > Cc: Ram Moon, AnandX 
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Add a check for object 
> > size for corner cases
> > 
> > Quoting Anand Moon (2021-02-10 07:59:29)
> > > Add check for object size to return appropriate error -E2BIG or 
> > > -EINVAL to avoid WARM_ON and successful return for some testcase.
> > 
> > No. You miss the point of having those warnings. We need to inspect the 
> > code to remove the last remaining "int pagenum", and then we can remove the 
> > GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX). These are not emitted for users, 
> > only for us to motivate us into finally fixing the code.
> > -Chris
> > 
> > Yes, I got your point these check are meant to catch up integer overflow.
> > 
> > I have check with the IGT testcase case  _gem_create_ and 
> > _gem_userptr_blits_ which fails pass size *-1ull << 32*  left shift 
> > and *0~* which leads to integer overflow ie  _negative_ size passed to 
> > create  i915_gem_create via ioctl  this leads to GM_WARN_ON.
> > 
> > Can we drop these testcase so that we don't break the kernel ?
> 
> The kernel rejects the ioctl with the expected errno. We leave a warning 
> purely for the benefit of CI, only when compiled for debugging and not by 
> default, so that we have a persistent reminder to do the code review.
> What's broken?
> -Chris
> 
> All though the testcase return with appropriate error we observe kernel taint 
> see below.

Which is an intentional taint added for CI so that we get a warning and
a visible bug so that we can allocate resources to _fix_ the underlying
problems in the code.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panel: elida-kd35t133: allow using non-continuous dsi clock

[Chris Morgan]

This fixes an issue with the panel not working after
commit c6d94e37bdbb ("drm/bridge/synopsys: dsi: add support for non-continuous 
HS clock").
With this change the panel inits successfully and displays an image.

Signed-off-by: Chris Morgan 
---
 drivers/gpu/drm/panel/panel-elida-kd35t133.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c 
b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index bc36aa3c1123..fe5ac3ef9018 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
@@ -265,7 +265,8 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
dsi->lanes = 1;
dsi->format = MIPI_DSI_FMT_RGB888;
dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
- MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_EOT_PACKET;
+ MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_EOT_PACKET |
+ MIPI_DSI_CLOCK_NON_CONTINUOUS;
 
drm_panel_init(&ctx->panel, &dsi->dev, &kd35t133_funcs,
   DRM_MODE_CONNECTOR_DSI);
-- 
2.25.1

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

[PATCH] dma-buf: Fix confusion of dynamic dma-buf vs dynamic attachment

[Chris Wilson]

Commit c545781e1c55 ("dma-buf: doc polish for pin/unpin") disagrees with
the introduction of dynamism in commit: bb42df4662a4 ("dma-buf: add
dynamic DMA-buf handling v15") resulting in warning spew on
importing dma-buf. Silence the warning from the latter by only pinning
the attachment if the attachment rather than the dmabuf is to be
dynamic.

Fixes: bb42df4662a4 ("dma-buf: add dynamic DMA-buf handling v15")
Fixes: c545781e1c55 ("dma-buf: doc polish for pin/unpin")
Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
Cc: Christian König 
Cc:  # v5.7+
---
 drivers/dma-buf/dma-buf.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f264b70c383e..09f5ae458515 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -758,8 +758,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct 
device *dev,
dma_buf_is_dynamic(dmabuf)) {
struct sg_table *sgt;
 
-   if (dma_buf_is_dynamic(attach->dmabuf)) {
-   dma_resv_lock(attach->dmabuf->resv, NULL);
+   if (dma_buf_attachment_is_dynamic(attach)) {
+   dma_resv_lock(dmabuf->resv, NULL);
ret = dma_buf_pin(attach);
if (ret)
goto err_unlock;
@@ -772,8 +772,9 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct 
device *dev,
ret = PTR_ERR(sgt);
goto err_unpin;
}
-   if (dma_buf_is_dynamic(attach->dmabuf))
-   dma_resv_unlock(attach->dmabuf->resv);
+   if (dma_buf_attachment_is_dynamic(attach))
+   dma_resv_unlock(dmabuf->resv);
+
attach->sgt = sgt;
attach->dir = DMA_BIDIRECTIONAL;
}
-- 
2.20.1

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

Re: [PATCH] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

[Chris Wilson]

Quoting Jason Ekstrand (2021-03-05 17:05:46)
> This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> since that commit, we've been having issues where a hang in one client
> can propagate to another.  In particular, a hang in an app can propagate
> to the X server which causes the whole desktop to lock up.

The fence error handling is required to prevent user's circumventing
incomplete work, such as security validation or escaping isolation.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gpu: drm: i915: fix error return code of igt_threaded_blt()

[Chris Wilson]

Quoting Jia-Ju Bai (2021-03-08 09:07:22)
> When kcalloc() returns NULL to tsk or thread, no error code of 
> igt_threaded_blt() is returned.
> To fix this bug, -ENOMEM is returned as error code.

Because we decided to skip the test if it could not be run due to
insufficient memory, as opposed to an internal allocation failure from
the driver.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gpu: drm: i915: fix error return code of igt_buddy_alloc_smoke()

[Chris Wilson]

Quoting Jia-Ju Bai (2021-03-08 08:59:52)
> When i915_random_order() returns NULL to order, no error return code of
> igt_buddy_alloc_smoke() is assigned.
> To fix this bug, err is assigned with -EINVAL in this case.

It would not be EINVAL since that is used for a reference failure, but
in this case the idea was to return 0 as no testing was done and the
ENOMEM was raised before testing began i.e. not an internal and
unexpected driver allocation failure.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

[Chris Wilson]

Quoting Daniel Vetter (2021-03-11 16:01:46)
> On Fri, Mar 05, 2021 at 11:05:46AM -0600, Jason Ekstrand wrote:
> > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
> > since that commit, we've been having issues where a hang in one client
> > can propagate to another.  In particular, a hang in an app can propagate
> > to the X server which causes the whole desktop to lock up.
> > 
> > Signed-off-by: Jason Ekstrand 
> > Reported-by: Marcin Slusarz 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
> > Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already 
> > signaled fences")
> 
> Yeah I suggested to just go with the revert, so I guess on my to give you
> the explainer to be added to the commit message.

If you took the patch this was copied from and only revert on the
dma-resv, things do not break horribly.

> Error propagation along fences sound like a good idea, but as your bug
> shows, surprising consequences, since propagating errors across security
> boundaries is not a good thing.
> 
> What we do have is track the hangs on the ctx, and report information to
> userspace using RESET_STATS.

And by the fence, which is far more precise.

> That's how arb_robustness works. Also, if my
> understanding is still correct, the EIO from execbuf is when your context
> is banned (because not recoverable or too many hangs). And in all these
> cases it's up to userspace to figure out what is all impacted and should
> be reported to the application, that's not on the kernel to guess and
> automatically propagate.
> 
> What's more, we're also building more features on top of ctx error
> reporting with RESET_STATS ioctl: Encrypted buffers use the same, and the
> userspace fence wait also relies on that mechanism. So it is the path
> going forward for reporting gpu hangs and resets to userspace.

That ioctl is not a solid basis, it never did quite work as expected and
we kept realising the limitations of the information and the accuracy
that it could report.
 
> So all together that's why I think we should just bury this idea again as
> not quite the direction we want to go to, hence why I think the revert is
> the right option here.

No, as shown by igt it's a critical issue that we have to judicially
chose which errors to ignore. And it's not just the ability to subvert
gen7 and gen9, it's the error tracking employed for preempting contexts
among others.  Hence go with the original patch to undo the propagation
along dma-resv.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] i915: Drop relocation support on all new hardware (v3)

[Chris Wilson]

Quoting Zbigniew Kempczyński (2021-03-11 11:44:32)
> On Wed, Mar 10, 2021 at 03:50:07PM -0600, Jason Ekstrand wrote:
> > The Vulkan driver in Mesa for Intel hardware never uses relocations if
> > it's running on a version of i915 that supports at least softpin which
> > all versions of i915 supporting Gen12 do.  On the OpenGL side, Gen12+ is
> > only supported by iris which never uses relocations.  The older i965
> > driver in Mesa does use relocations but it only supports Intel hardware
> > through Gen11 and has been deprecated for all hardware Gen9+.  The
> > compute driver also never uses relocations.  This only leaves the media
> > driver which is supposed to be switching to softpin going forward.
> > Making softpin a requirement for all future hardware seems reasonable.
> > 
> > Rejecting relocations starting with Gen12 has the benefit that we don't
> > have to bother supporting it on platforms with local memory.  Given how
> > much CPU touching of memory is required for relocations, not having to
> > do so on platforms where not all memory is directly CPU-accessible
> > carries significant advantages.
> > 
> > v2 (Jason Ekstrand):
> >  - Allow TGL-LP platforms as they've already shipped
> > 
> > v3 (Jason Ekstrand):
> >  - WARN_ON platforms with LMEM support in case the check is wrong
> 
> I was asked to review of this patch. It works along with expected
> IGT check https://patchwork.freedesktop.org/patch/423361/?series=82954&rev=25
> 
> Before I'll give you r-b - isn't i915_gem_execbuffer2_ioctl() better place
> to do for loop just after copy_from_user() and check relocation_count?
> We have an access to exec2_list there, we know the gen so we're able to say
> relocations are not supported immediate, without entering 
> i915_gem_do_execbuffer().

There's a NORELOC flag you can enforce as mandatory. That's trivial for
userspace to set, really makes sure they are aware of the change afoot,
and i915_gem_ceck_execbuffer() will perform the validation upfront with
the other flag checks.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

NVIDIA GPU fallen off the bus after exiting s2idle

[Chris Chiu]

Hi,
We have some Intel laptops (11th generation CPU) with NVIDIA GPU
suffering the same GPU falling off the bus problem while exiting
s2idle with external display connected. These laptops connect the
external display via the HDMI/DisplayPort on a USB Type-C interfaced
dock. If we enter and exit s2idle with the dock connected, the NVIDIA
GPU (confirmed on 10de:24b6 and 10de:25b8) and the PCIe port can come
back to D0 w/o problem. If we enter the s2idle, disconnect the dock,
then exit the s2idle, both external display and the panel will remain
with no output. The dmesg as follows shows the "nvidia :01:00.0:
can't change power state from D3cold to D0 (config space
inaccessible)" due to the following ACPI error
[ 154.446781]
[ 154.446783]
[ 154.446783] Initialized Local Variables for Method [IPCS]:
[ 154.446784] Local0: 9863e365  Integer 09C5
[ 154.446790]
[ 154.446791] Initialized Arguments for Method [IPCS]: (7 arguments
defined for method invocation)
[ 154.446792] Arg0: 25568fbd  Integer 00AC
[ 154.446795] Arg1: 9ef30e76  Integer 
[ 154.446798] Arg2: fdf820f0  Integer 0010
[ 154.446801] Arg3: 9fc2a088  Integer 0001
[ 154.446804] Arg4: 3a3418f7  Integer 0001
[ 154.446807] Arg5: 20c4b87c  Integer 
[ 154.446810] Arg6: 8b965a8a  Integer 
[ 154.446813]
[ 154.446815] ACPI Error: Aborting method \IPCS due to previous error
(AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446824] ACPI Error: Aborting method \MCUI due to previous error
(AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446829] ACPI Error: Aborting method \SPCX due to previous error
(AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446835] ACPI Error: Aborting method \_SB.PC00.PGSC due to
previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446841] ACPI Error: Aborting method \_SB.PC00.PGON due to
previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446846] ACPI Error: Aborting method \_SB.PC00.PEG1.NPON due to
previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446852] ACPI Error: Aborting method \_SB.PC00.PEG1.PG01._ON due
to previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
[ 154.446860] acpi device:02: Failed to change power state to D0
[ 154.690760] video LNXVIDEO:00: Cannot transition to power state D0
for parent in (unknown)

The IPCS is the last function called from \_SB.PC00.PEG1.PG01._ON
which we expect it to prepare everything before bringing back the
NVIDIA GPU but it's stuck in the infinite loop as described below.
Please refer to
https://gist.github.com/mschiu77/fa4f5a97297749d0d66fe60c1d421c44 for
the full DSDT.dsl.
   While (One)
{
If ((!IBSY || (IERR == One)))
{
Break
}

If ((Local0 > TMOV))
{
RPKG [Zero] = 0x03
Return (RPKG) /* \IPCS.RPKG */
}

Sleep (One)
Local0++
}

And the upstream PCIe port of NVIDIA seems to become inaccessible due
to the messages as follows.
[ 292.746508] pcieport :00:01.0: waiting 100 ms for downstream
link, after activation
[ 292.882296] pci :01:00.0: waiting additional 100 ms to become accessible
[ 316.876997] pci :01:00.0: can't change power state from D3cold
to D0 (config space inaccessible)

Since the IPCS is the Intel Reference Code and we don't really know
why the never-end loop happens just because we unplug the dock while
the system still stays in s2idle. Can anyone from Intel suggest what
happens here?

And one thing also worth mentioning, if we unplug the display cable
from the dock before entering the s2idle, NVIDIA GPU can come back w/o
problem even if we disconnect the dock before exiting s2idle. Here's
the lspci information
https://gist.github.com/mschiu77/0bfc439d15d52d20de0129b1b2a86dc4 and
the dmesg log with ACPI trace_state enabled and dynamic debug on for
drivers/pci/pci.c, drivers/acpi/device_pm.c for the whole s2idle
enter/exit with IPCS timeout.

Any suggestion would be appreciated. Thanks.

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

Re: NVIDIA GPU fallen off the bus after exiting s2idle

[Chris Chiu]

On Thu, May 6, 2021 at 5:46 PM Rafael J. Wysocki  wrote:
>
> On Tue, May 4, 2021 at 10:08 AM Chris Chiu  wrote:
> >
> > Hi,
> > We have some Intel laptops (11th generation CPU) with NVIDIA GPU
> > suffering the same GPU falling off the bus problem while exiting
> > s2idle with external display connected. These laptops connect the
> > external display via the HDMI/DisplayPort on a USB Type-C interfaced
> > dock. If we enter and exit s2idle with the dock connected, the NVIDIA
> > GPU (confirmed on 10de:24b6 and 10de:25b8) and the PCIe port can come
> > back to D0 w/o problem. If we enter the s2idle, disconnect the dock,
> > then exit the s2idle, both external display and the panel will remain
> > with no output. The dmesg as follows shows the "nvidia :01:00.0:
> > can't change power state from D3cold to D0 (config space
> > inaccessible)" due to the following ACPI error
> > [ 154.446781]
> > [ 154.446783]
> > [ 154.446783] Initialized Local Variables for Method [IPCS]:
> > [ 154.446784] Local0: 9863e365  Integer 09C5
> > [ 154.446790]
> > [ 154.446791] Initialized Arguments for Method [IPCS]: (7 arguments
> > defined for method invocation)
> > [ 154.446792] Arg0: 25568fbd  Integer 00AC
> > [ 154.446795] Arg1: 9ef30e76  Integer 
> > [ 154.446798] Arg2: fdf820f0  Integer 0010
> > [ 154.446801] Arg3: 9fc2a088  Integer 0001
> > [ 154.446804] Arg4: 3a3418f7  Integer 0001
> > [ 154.446807] Arg5: 20c4b87c  Integer 
> > [ 154.446810] Arg6: 8b965a8a  Integer 
> > [ 154.446813]
> > [ 154.446815] ACPI Error: Aborting method \IPCS due to previous error
> > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446824] ACPI Error: Aborting method \MCUI due to previous error
> > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446829] ACPI Error: Aborting method \SPCX due to previous error
> > (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446835] ACPI Error: Aborting method \_SB.PC00.PGSC due to
> > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446841] ACPI Error: Aborting method \_SB.PC00.PGON due to
> > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446846] ACPI Error: Aborting method \_SB.PC00.PEG1.NPON due to
> > previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446852] ACPI Error: Aborting method \_SB.PC00.PEG1.PG01._ON due
> > to previous error (AE_AML_LOOP_TIMEOUT) (20200925/psparse-529)
> > [ 154.446860] acpi device:02: Failed to change power state to D0
> > [ 154.690760] video LNXVIDEO:00: Cannot transition to power state D0
> > for parent in (unknown)
>
> If I were to guess, I would say that AML tries to access memory that
> is not accessible while suspended, probably PCI config space.
>
> > The IPCS is the last function called from \_SB.PC00.PEG1.PG01._ON
> > which we expect it to prepare everything before bringing back the
> > NVIDIA GPU but it's stuck in the infinite loop as described below.
> > Please refer to
> > https://gist.github.com/mschiu77/fa4f5a97297749d0d66fe60c1d421c44 for
> > the full DSDT.dsl.
>
> The DSDT alone may not be sufficient.
>
> Can you please create a bug entry at bugzilla.kernel.org for this
> issue and attach the full output of acpidump from one of the affected
> machines to it?  And please let me know the number of the bug.
>
> Also please attach the output of dmesg including a suspend-resume
> cycle including dock disconnection while suspended and the ACPI
> messages quoted below.
>
> >While (One)
> > {
> > If ((!IBSY || (IERR == One)))
> > {
> > Break
> > }
> >
> > If ((Local0 > TMOV))
> > {
> > RPKG [Zero] = 0x03
> > Return (RPKG) /* \IPCS.RPKG */
> > }
> >
> > Sleep (One)
> > Local0++
> > }
> >
> > And the upstream PCIe port of NVIDIA seems to become inaccessible due
> > to the messages as follows.
> > [ 292.746508] pcieport :00:01.0: waiting 100 ms for downstream
> > link, after activation
> > [ 292.882296] pci :01:00.0: waiting additional 100 ms to become 
> > accessible
> > [ 316.876997] pci :01:00.0: can't change power state from D3cold
> > to D0 (config space

Re: [Intel-gfx] [PATCH] drm/i915: fix error return code in check_partial_mapping()

[Chris Wilson]

Quoting Luo Meng (2020-11-25 01:29:38)
> Fix to return a negative error code from the error handling case
> instead of 0 in function check_partial_mapping(), as done elsewhere
> in this function.

It's not an error, just the end of testing.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/modes: Switch to 64bit maths to avoid integer overflow

[Chris Wilson]

Quoting Ville Syrjälä (2020-10-30 14:43:46)
> On Fri, Oct 30, 2020 at 02:19:45PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2020-10-22 20:42:56)
> > > From: Ville Syrjälä 
> > > 
> > > The new >8k CEA modes have dotclocks reaching 5.94 GHz, which
> > > means our clock*1000 will now overflow the 32bit unsigned
> > > integer. Switch to 64bit maths to avoid it.
> > > 
> > > Cc: sta...@vger.kernel.org
> > > Reported-by: Randy Dunlap 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > > An interesting question how many other place might suffer from similar
> > > overflows. I think i915 should be mostly OK. The one place I know we use
> > > Hz instead kHz is the hsw DPLL code, which I would prefer we also change
> > > to use kHz. The other concern is whether we have any potential overflows
> > > before we check this against the platform's max dotclock.
> > > 
> > > I do have this unreviewed igt series 
> > > https://patchwork.freedesktop.org/series/69531/ which extends the
> > > current testing with some other forms of invalid modes. Could probably
> > > extend that with a mode.clock=INT_MAX test to see if anything else might
> > > trip up.
> > > 
> > > No idea about other drivers.
> > > 
> > >  drivers/gpu/drm/drm_modes.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > > index 501b4fe55a3d..511cde5c7fa6 100644
> > > --- a/drivers/gpu/drm/drm_modes.c
> > > +++ b/drivers/gpu/drm/drm_modes.c
> > > @@ -762,7 +762,7 @@ int drm_mode_vrefresh(const struct drm_display_mode 
> > > *mode)
> > > if (mode->htotal == 0 || mode->vtotal == 0)
> > > return 0;
> > >  
> > > -   num = mode->clock * 1000;
> > > +   num = mode->clock;
> > > den = mode->htotal * mode->vtotal;
> > 
> > You don't want to promote den to u64 while you are here? We are at
> > 8kx4k, throw in dblscan and some vscan, and we could soon have wacky
> > refresh rates.
> 
> i915 has 16kx8k hard limit currently, and we reject vscan>1
> (wish we could also reject DBLSCAN). So we should not hit
> that, at least not yet. Other drivers might not be so strict
> I guess.
> 
> I have a nagging feeling that other places are in danger of
> overflows if we try to push the current limits significantly.
> But I guess no real harm in going full 64bit here, except
> maybe making it a bit slower.

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

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

 the ABI (e.g. mmap) we think in system pages */
> +   GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
> +
> +   /* Allocate the new object */
> +   obj = i915_gem_object_create_region(placements[0], size, 0);
> +   if (IS_ERR(obj))
> +   return PTR_ERR(obj);
> +
> +   if (i915_gem_object_is_lmem(obj)) {
> +   struct intel_gt *gt = obj->mm.region->gt;
> +   struct intel_context *ce = gt->engine[BCS0]->blitter_context;
> +
> +   /*
> +* XXX: We really want to move this to get_pages(), but we
> +* require grabbing the BKL for the blitting operation which 
> is
> +* annoying. In the pipeline is support for async get_pages()
> +* which should fit nicely for this. Also note that the actual
> +* clear should be done async(we currently do an object_wait
> +* which is pure garbage), we just need to take care if
> +* userspace opts of implicit sync for the execbuf, to avoid 
> any
> +* potential info leak.
> +*/

Not just XXX, but the design should be completed first.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 093/162] drm/i915/lmem: allocate cmd ring in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:09)
> From: Michel Thierry 
> 
> Signed-off-by: Michel Thierry 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> ---
>  drivers/gpu/drm/i915/gt/intel_ring.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c 
> b/drivers/gpu/drm/i915/gt/intel_ring.c
> index d636c6ed88b7..aa75e644f3f2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c
> @@ -4,6 +4,7 @@
>   * Copyright © 2019 Intel Corporation
>   */
>  
> +#include "gem/i915_gem_lmem.h"
>  #include "gem/i915_gem_object.h"
>  #include "i915_drv.h"
>  #include "i915_vma.h"
> @@ -111,10 +112,16 @@ static struct i915_vma *create_ring_vma(struct 
> i915_ggtt *ggtt, int size)
> struct i915_vma *vma;
>  
> obj = ERR_PTR(-ENODEV);
> -   if (i915_ggtt_has_aperture(ggtt))
> -   obj = i915_gem_object_create_stolen(i915, size);
> -   if (IS_ERR(obj))
> -   obj = i915_gem_object_create_internal(i915, size);
> +   if (HAS_LMEM(i915)) {
> +   obj = i915_gem_object_create_lmem(i915, size,
> + I915_BO_ALLOC_CONTIGUOUS |
> + I915_BO_ALLOC_VOLATILE);

Just create, and keep trying when !lmem returns an error.

Why contiguous, it's vmapped anyway?

> +   } else {
> +   if (i915_ggtt_has_aperture(ggtt))
> +   obj = i915_gem_object_create_stolen(i915, size);
> +   if (IS_ERR(obj))
> +   obj = i915_gem_object_create_internal(i915, size);
> +   }
> if (IS_ERR(obj))
> return ERR_CAST(obj);
>  
> -- 
> 2.26.2
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 097/162] drm/i915: Distinction of memory regions

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:13)
> From: Zbigniew Kempczyński 
> 
> IGTs should be able to choose testing strategy depending on memory
> regions and its sizes. Add region instance number to make this
> easier and descriptive.
> 
> Cc: Matthew Auld 
> Cc: Ramalingam C 
> Cc: Tvrtko Ursulin 
> Cc: Adam Miszczak 
> Signed-off-by: Zbigniew Kempczyński 
> ---
>  drivers/gpu/drm/i915/intel_memory_region.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c 
> b/drivers/gpu/drm/i915/intel_memory_region.c
> index 1f26bc06ec20..cea44ddebe46 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -329,6 +329,10 @@ int intel_memory_regions_hw_probe(struct 
> drm_i915_private *i915)
> mem->instance = instance;
> mem->gt = &i915->gt;
>  
> +   if (HAS_LMEM(mem->i915) && type != INTEL_MEMORY_SYSTEM)
> +   intel_memory_region_set_name(mem, "%s%u",
> +    mem->name, 
> mem->instance);

sprintf(mem->name, "%s", mem->name)

is that even defined behaviour?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 098/162] drm/i915/gtt: map the PD up front

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:14)
> We need to general our accessor for the page directories and tables from
> using the simple kmap_atomic to support local memory, and this setup
> must be done on acquisition of the backing storage prior to entering
> fence execution contexts. Here we replace the kmap with the object
> maping code that for simple single page shmemfs object will return a
> plain kmap, that is then kept for the lifetime of the page directory.
> 
> Signed-off-by: Matthew Auld 
> Signed-off-by: Chris Wilson 

We are going to really struggle with this on 32b :(
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 101/162] drm/i915/gtt/dg1: add PTE_LM plumbing for PPGTT

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:17)
> For the PTEs we get an LM bit, to signal whether the page resides in
> SMEM or LMEM.
> 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> Signed-off-by: Daniele Ceraolo Spurio 
> Signed-off-by: Niranjana Vishwanathapura 
> Signed-off-by: Venkata Sandeep Dhanalakota 
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c  | 35 ++-
>  drivers/gpu/drm/i915/gt/intel_gtt.h   |  3 +++
>  drivers/gpu/drm/i915/gt/intel_ppgtt.c |  4 +++
>  3 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index e2f1dfc48d43..b6fcebeef02a 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -5,6 +5,7 @@
>  
>  #include 
>  
> +#include "gem/i915_gem_lmem.h"
>  #include "gen8_ppgtt.h"
>  #include "i915_scatterlist.h"
>  #include "i915_trace.h"
> @@ -50,6 +51,21 @@ static u64 gen8_pte_encode(dma_addr_t addr,
> return pte;
>  }
>  
> +static u64 gen12_pte_encode(dma_addr_t addr,
> +   enum i915_cache_level level,
> +   u32 flags)
> +{
> +   gen8_pte_t pte = addr | _PAGE_PRESENT | _PAGE_RW;
> +
> +   if (unlikely(flags & PTE_READ_ONLY))
> +   pte &= ~_PAGE_RW;
> +
> +   if (flags & PTE_LM)
> +   pte |= GEN12_PPGTT_PTE_LM;
> +
> +   return pte;
> +}
> +
>  static void gen8_ppgtt_notify_vgt(struct i915_ppgtt *ppgtt, bool create)
>  {
> struct drm_i915_private *i915 = ppgtt->vm.i915;
> @@ -365,7 +381,7 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
>   u32 flags)
>  {
> struct i915_page_directory *pd;
> -   const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
> +   const gen8_pte_t pte_encode = ppgtt->vm.pte_encode(0, cache_level, 
> flags);

We don't need the vfunc, since that flag will not be sent for gen8.

That bit test will be cheaper than the retpoline.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 103/162] drm/i915: allocate context from LMEM

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:19)
> Based on a patch from Michel Thierry.
> 
> Signed-off-by: Matthew Auld 
> Cc: Joonas Lahtinen 
> Cc: Abdiel Janulgue 
> ---
>  .../drm/i915/gt/intel_execlists_submission.c  | 31 ++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c 
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 582a9044727e..c640b90711fd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -108,6 +108,8 @@
>   */
>  #include 
>  
> +#include "gem/i915_gem_lmem.h"
> +
>  #include "i915_drv.h"
>  #include "i915_perf.h"
>  #include "i915_trace.h"
> @@ -4660,6 +4662,21 @@ static struct intel_timeline *pinned_timeline(struct 
> intel_context *ce)
>  page_unmask_bits(tl));
>  }
>  
> +static int context_clear_lmem(struct drm_i915_gem_object *ctx_obj)
> +{
> +   void *vaddr;
> +
> +   vaddr = i915_gem_object_pin_map(ctx_obj, I915_MAP_WC);
> +   if (IS_ERR(vaddr))
> +   return PTR_ERR(vaddr);
> +
> +   memset64(vaddr, 0, ctx_obj->base.size / sizeof(u64));
> +
> +   i915_gem_object_unpin_map(ctx_obj);

What? We copy over the entire object with the default state.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 118/162] drm/i915/dg1: Reserve first 1MB of local memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:34)
> From: Imre Deak 
> 
> On DG1 A0/B0 steppings the first 1MB of local memory must be reserved.
> One reason for this is that the 0xA-0xB range is not accessible
> by the display, probably since this region is redirected to another
> memory location for legacy VGA compatibility.
> 
> BSpec: 50586
> Testcase: igt/kms_big_fb/linear-64bpp-rotate-0
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_region_lmem.c | 52 
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> b/drivers/gpu/drm/i915/intel_region_lmem.c
> index 939cf0d195a5..eafef7034680 100644
> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
> return mem;
>  }
>  
> +static void get_legacy_lowmem_region(struct intel_uncore *uncore,
> +u64 *start, u32 *size)
> +{
> +   *start = 0;
> +   *size = 0;
> +
> +   if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0))
> +   return;
> +
> +   *size = SZ_1M;
> +
> +   DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory [0x%llx-0x%llx]\n",
> +*start, *start + *size);
> +}
> +
> +static int reserve_lowmem_region(struct intel_uncore *uncore,
> +struct intel_memory_region *mem)
> +{
> +   u64 reserve_start;
> +   u64 reserve_end;
> +   u64 region_start;
> +   u32 region_size;
> +   int ret;
> +
> +   get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size);
> +   reserve_start = region_start;
> +   reserve_end = region_start + region_size;
> +
> +   if (!reserve_end)
> +   return 0;
> +
> +   DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n",
> +reserve_start, reserve_end);
> +   ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved,
> +    reserve_start,
> +reserve_end - reserve_start);

Isn't this now relative to the stolen offset? Should this be reserved,
or excluded like stolen?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 124/162] drm/i915/lmem: allocate HWSP in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:40)
> From: Michel Thierry 

Rationale goes here.

Is this wise? HWSP is very frequently read by the CPU, and expected to
be cached on the CPU.

What do the performance profiles indicate?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 125/162] drm/i915/lmem: Limit block size to 4G

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:41)
> From: Venkata Sandeep Dhanalakota 
> 
> when allocating pages to lmem object of size 4G or greater
> we allocate memory blocks from buddy system.

Any lmem object is from the buddy system.

> In this scenario
> buddy sytem can allocate blocks that can have size >= 4G and
> these blocks require >32b to represent block size with these
> blocks we run into an issue with sg list construction because
> sg->length field is only 32b wide.

Just say the when using scatterlist, the maximum segment size is 4G. In
fact, we can ask sg what the backend maximum is, and use that as our max
order.

The only question is whether this merits a flag, or we just assume that
the buddy allocator is only used for objects and so always presented via
sg?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 126/162] drm/i915/gem: Update shmem available memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:42)
> From: Bommu Krishnaiah 
> 
> Update shmem available memory in “intel_memory_region”

Was avail ever set?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 128/162] drm/i915/dg1: intel_memory_region_evict() changes for eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:44)
> From: CQ Tang 
> 
> Function i915_gem_shrink_memory_region() is changed to
> intel_memory_region_evict() and moved from i915_gem_shrinker.c
> to intel_memory_region.c, this function is used to handle local
> memory swapping, in addition to evict purgeable objects only.

We really do not want to conflate the system shrinker with eviction.
Reservation based eviction looks nothing like the shrinker.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 133/162] drm/i915/dg1: Track swap in/out stats via debugfs

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:49)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1366b53ac8c9..7b1e95d494e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1214,6 +1214,9 @@ struct drm_i915_private {
>  * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your 
> patch
>  * will be rejected. Instead look for a better place.
>  */
> +
> +   atomic_long_t num_bytes_swapped_out;
> +   atomic_long_t num_bytes_swapped_in;

Enough said. Don't mindlessly add fields.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 134/162] drm/i915/dg1: Measure swap in/out timing stats

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:50)
> From: Sudeep Dutt 
> 
> Signed-off-by: Sudeep Dutt 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_region.c | 16 ++--
>  drivers/gpu/drm/i915/i915_debugfs.c|  3 +++
>  drivers/gpu/drm/i915/i915_drv.h|  2 ++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index ed108dbcb34e..4fab9f6b4bee 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -15,6 +15,7 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
>  {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> struct drm_i915_gem_object *dst, *src;
> +   unsigned long start, diff, msec;
> int err;
>  
> GEM_BUG_ON(obj->swapto);
> @@ -24,6 +25,7 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
> GEM_BUG_ON(!i915->params.enable_eviction);
>  
> assert_object_held(obj);
> +   start = jiffies;
>  
> /* create a shadow object on smem region */
> dst = i915_gem_object_create_shmem(i915, obj->base.size);
> @@ -64,8 +66,12 @@ i915_gem_object_swapout_pages(struct drm_i915_gem_object 
> *obj,
> else
> i915_gem_object_put(dst);
>  
> -   if (!err)
> +   if (!err) {
> +   diff = jiffies - start;
> +   msec = diff * 1000 / HZ;
> +   atomic_long_add(msec, &i915->time_swap_out_ms);
> atomic_long_add(sizes, &i915->num_bytes_swapped_out);
> +   }

This can be done using a kprobe, and with prettier statistics as builtin
functionality.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 137/162] drm/i915: blt copy between objs using pre-created vma windows

[Chris Wilson]

> +   blt_copied += cur_win_sz;
> +   err = 0;
> +   i915_request_put(rq);
> +   flush_work(&i915->gt.engine[BCS0]->retire_work);

Papering (doubtful the paper is successful) over bugs by introducing a
whole load more.

This fails the basic premise that eviction must be pipelined. The PTE
are transient and can be written prior to the copy and kept within the
non-preemptible window of the blt. Thus allowing many evictions to
scheduled in parallel (by either allocating separate contexts, or more
preferably picking a user context).
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 140/162] drm/i915: window_blt_copy is used for swapin and swapout

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:56)
> From: Ramalingam C 
> 
> window_blt_copy feature is used for swapin and swapout based on the i915
> module parameter called enable_eviction.

A module parameter?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 141/162] drm/i915: Lmem eviction statistics by category

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:57)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 82f431cc38cd..6f0ab363bdee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1225,6 +1225,11 @@ struct drm_i915_private {
> atomic_long_t num_bytes_swapped_in;
> atomic_long_t time_swap_out_ms;
> atomic_long_t time_swap_in_ms;
> +
> +   atomic_long_t num_bytes_swapped_out_memcpy;
> +   atomic_long_t num_bytes_swapped_in_memcpy;
> +   atomic_long_t time_swap_out_ms_memcpy;
> +   atomic_long_t time_swap_in_ms_memcpy;

See earlier comments about why this will be rejected.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 143/162] drm/i915: suspend/resume eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:59)
> +static int intel_dmem_evict_buffers(struct drm_device *dev, bool in_suspend)
> +{
> +   struct drm_i915_private *i915 = to_i915(dev);
> +   struct drm_i915_gem_object *obj;
> +   struct intel_memory_region *mem;
> +   int id, ret = 0;
> +
> +   /*
> +* FIXME: Presently using memcpy,
> +* will replace with blitter once
> +* fix the issues.
> +*/

Why hasn't it been fixed then?
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 144/162] drm/i915: Reset blitter context when unpark engine

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:00)
> From: Venkata Ramana Nayana 
> 
> We are only doing it now for kernel_context. We also need to do for the
> copy engine  blitter context.
> 
> Signed-off-by: Venkata Ramana Nayana 
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_pm.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> index 1b2009b4dcb7..69c8ea70d1e8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -66,6 +66,11 @@ static int __engine_unpark(struct intel_wakeref *wf)
> ce->ops->reset(ce);
> }

Add a list of pinned volatile contexts to the engine that must be
restored across resume.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 146/162] drm/i915/pm: suspend and restore ppgtt mapping

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:02)
> From: Prathap Kumar Valsan 
> 
> During suspend we will lose all page tables as they are allocated in
> LMEM. In-order to  make sure that the contexts do not access the
> corrupted page table after we restore, we are evicting all vma's that
> are bound to vm's. This includes kernel vm.
> 
> During resume, we are restoring the page tables back to scratch page.
> 
> Signed-off-by: Prathap Kumar Valsan 
> Signed-off-by: Venkata Ramana Nayana 
> Cc: CQ Tang 
> ---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c  |  13 
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.h  |   2 +
>  drivers/gpu/drm/i915/gt/intel_ppgtt.c |   4 +
>  drivers/gpu/drm/i915/i915_drv.c   | 102 +++---
>  4 files changed, 112 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index b6fcebeef02a..704cab807e0b 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -775,3 +775,16 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt)
> kfree(ppgtt);
> return ERR_PTR(err);
>  }
> +
> +void gen8_restore_ppgtt_mappings(struct i915_address_space *vm)
> +{
> +   const unsigned int count = gen8_pd_top_count(vm);
> +   int i;
> +
> +   for (i = 1; i <= vm->top; i++)
> +   fill_px(vm->scratch[i], vm->scratch[i - 1]->encode);
> +
> +   fill_page_dma(px_base(i915_vm_to_ppgtt(vm)->pd),
> + vm->scratch[vm->top]->encode, count);
> +}
> +
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h 
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> index 76a08b9c1f5c..3fa4b95aaabd 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.h
> @@ -6,8 +6,10 @@
>  #ifndef __GEN8_PPGTT_H__
>  #define __GEN8_PPGTT_H__
>  
> +struct i915_address_space;
>  struct intel_gt;
>  
> +void gen8_restore_ppgtt_mappings(struct i915_address_space *vm);
>  struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt);
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_ppgtt.c 
> b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> index 34a02643bb75..9b3eacd12a7e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ppgtt.c
> @@ -9,6 +9,8 @@
>  #include "intel_gtt.h"
>  #include "gem/i915_gem_lmem.h"
>  #include "gem/i915_gem_region.h"
> +#include "gem/i915_gem_context.h"
> +#include "gem/i915_gem_region.h"
>  #include "gen6_ppgtt.h"
>  #include "gen8_ppgtt.h"
>  
> @@ -317,3 +319,5 @@ void ppgtt_init(struct i915_ppgtt *ppgtt, struct intel_gt 
> *gt)
> ppgtt->vm.vma_ops.set_pages   = ppgtt_set_pages;
> ppgtt->vm.vma_ops.clear_pages = clear_pages;
>  }
> +
> +
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e8c4931fc818..7115f4db5043 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -64,6 +64,7 @@
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_ioctls.h"
>  #include "gem/i915_gem_mman.h"
> +#include "gt/gen8_ppgtt.h"
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_pm.h"
>  #include "gt/intel_rc6.h"
> @@ -1136,13 +1137,13 @@ static int intel_dmem_evict_buffers(struct drm_device 
> *dev, bool in_suspend)
>  
> mutex_unlock(&mem->objects.lock);
>  
> -   if (in_suspend)
> -   i915_gem_object_unbind(obj, 0);
> -
> if (in_suspend) {
> obj->swapto = NULL;
> obj->evicted = false;
> obj->do_swapping = true;
> +
> +   i915_gem_object_unbind(obj, 0);
> +
> ret = 
> __i915_gem_object_put_pages(obj);
> obj->do_swapping = false;
> if (ret) {
> @@ -1176,6 +1177,43 @@ static int intel_dmem_evict_buffers(struct drm_device 
> *dev, bool in_suspend)
> return ret;
>  }
>  
> +static int i915_gem_suspend_ppgtt_mappings(struct drm_i915_private *i915)
> +{
> +   struct i915_gem_context *ctx, *cn;
> +   int ret;
> +
> +   spin_lock(&i915->gem.contexts.lock);
> +   list_for_each_entry_safe(ctx, cn, &i915->gem.contexts.list, link) {

Wrong list. Bad starting point from GEM.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 147/162] drm/i915/gt: Allocate default ctx objects in SMEM

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:03)
> From: Venkata Ramana Nayana 
> 
> If record default objects are created in LMEM and in suspend
> pin the pages of obj (src) and use blitter for eviction. But
> during request creation using blitter context and try to pin the same
> default object, to restore the ctx with default HW values, will leads to
> the dead lock situation. To avoid this, safe to keep these
> objects in SMEM.

Dead patch. Default object state should be recorded as shmemfs.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 148/162] drm/i915: suspend/resume enable blitter eviction

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:04)
> From: Venkata Ramana Nayana 
> 
> In suspend mode use blitter eviction before disable the runtime
> interrupts and in resume use blitter after the gem resume happens.

Consider add it to the suspend prepare function.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 150/162] drm/i915: need consider system BO snoop for dgfx

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:06)
> From: CQ Tang 
> 
> When cache_level is NONE, we check HAS_LLC(i915).
> But additionally for DGFX, we also need to check
> HAS_SNOOP(i915) on system memory object to use
> I915_BO_CACHE_COHERENT_FOR_READ. on dg1, has_llc=0, and
> has_snoop=1. Otherwise, we set obj->cache_choerent=0 and
> have performance impact.
> 
> Cc: Chris P Wilson 
> Cc: Ramalingam C 
> Cc: Sudeep Dutt 
> Cc: Matthew Auld 
> Signed-off-by: CQ Tang 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index ddb448f275eb..be603171c444 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -95,6 +95,20 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> mutex_init(&obj->mm.get_dma_page.lock);
>  }
>  
> +static bool i915_gem_object_use_llc(struct drm_i915_gem_object *obj)
> +{
> +   struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +
> +   if (HAS_LLC(i915))
> +   return true;
> +
> +   if (IS_DGFX(i915) && HAS_SNOOP(i915) &&
> +   !i915_gem_object_is_lmem(obj))
> +   return true;
> +
> +   return false;
> +}
> +
>  /**
>   * Mark up the object's coherency levels for a given cache_level
>   * @obj: #drm_i915_gem_object
> @@ -108,7 +122,7 @@ void i915_gem_object_set_cache_coherency(struct 
> drm_i915_gem_object *obj,
> if (cache_level != I915_CACHE_NONE)
> obj->cache_coherent = (I915_BO_CACHE_COHERENT_FOR_READ |
>I915_BO_CACHE_COHERENT_FOR_WRITE);
> -   else if (HAS_LLC(to_i915(obj->base.dev)))
> +   else if (i915_gem_object_use_llc(obj))
> obj->cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ;
> else
> obj->cache_coherent = 0;

You must also define obj->cache_level correctly. You can not just assume
the object will be snooped.
-Chris
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 157/162] drm/i915: Improve accuracy of eviction stats

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:13)
> From: Tvrtko Ursulin 
> 
> Current code uses jiffie time to do the accounting and then does:
> 
>   diff = jiffies - start;
>   msec = diff * 1000 / HZ;
>   ...
>   atomic_long_add(msec, &i915->time_swap_out_ms);
> 
> If we assume jiffie can be as non-granular as 10ms and that the current
> accounting records all evictions faster than one jiffie as infinite speed,
> we can end up over-estimating the reported eviction throughput.
> 
> Fix this by accumulating ktime_t and only dividing to more user friendly
> granularity at presentation time (debugfs read).
> 
> At the same time consolidate the code a bit and convert from multiple
> atomics to single seqlock per stat.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: CQ Tang 
> Cc: Sudeep Dutt 
> Cc: Mika Kuoppala 

A lot of effort to fix up patches after the fact, might as well make it
a real PMU interface.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 160/162] drm/i915/dg1: Fix GPU hang due to shmemfs page drop

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:16)
> From: Venkata Ramana Nayana 
> 
> This is to fix a bug in upstream
> commit a6326a4f8ffb ("drm/i915/gt: Keep a no-frills swappable copy of the 
> default context state")
> 
> We allocate context state obj ce->state from lmem, so in 
> __engines_record_defaults(),
> we call shmem_create_from_object(). Because it is lmem object, this call will
> create a new shmemfs file, copy the contents into it, and return the file
> pointer and assign to engine->default_state. Of course ce->state lmem object
> is freed at the end of function __engines_record_redefaults().
> 
> Because a new shmemfs file is create for engine->default_state,
> and more importantly, we DON'T mark the pages dirty after we write into it,
> the OS page cache eviction will drop these pages.
> 
> Now with the test move forward, it will create new request/context, and will
> copy the saved engine->default_state into ce->state. If the default_state
> pages are dropped during page cache eviction, the copying will get new pages,
> and copy garbage from the new pages. Next, ce->state will have wrong
> instruction and causes GPU to hang.
> 
> The fixing is very simple, we just mark the shmemfs pages to be dirty when
> writing into it, and also mark the pages to accessed when read/write to them.
> 
> Fixes: a6326a4f8ffb("drm/i915/gt: Keep a no-frills swappable copy of the 
> default context state")

A bug fix, send it. But please write a concise changelog first.

I missed setting the dirty bit, and so the contents were not being saved
on swap out as expected. Impact is severe; any context created after
resume may be gibberish.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:08)
> +int
> +i915_gem_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> +   struct drm_i915_private *i915 = to_i915(dev);
> +   struct create_ext ext_data = { .i915 = i915 };
> +   struct drm_i915_gem_create_ext *args = data;
> +   int ret;
> +
> +   i915_gem_flush_free_objects(i915);
> +
> +   ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
> +  create_extensions,
> +  ARRAY_SIZE(create_extensions),
> +  &ext_data);
> +   if (ret)
> +   goto err_free;
> +
> +   if (!ext_data.placements) {
> +   struct intel_memory_region **placements;
> +   enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
> +
> +   placements = kmalloc(sizeof(struct intel_memory_region *),
> +GFP_KERNEL);
> +   if (!placements)
> +   return -ENOMEM;
> +
> +   placements[0] = intel_memory_region_by_type(i915, mem_type);
> +
> +   ext_data.placements = placements;
> +   ext_data.n_placements = 1;
> +   }
> +
> +   ret = i915_gem_create(file,
> + ext_data.placements,
> + ext_data.n_placements,
> + &args->size, &args->handle);
> +   if (!ret)
> +   return 0;

Applying the extensions has to happen after creating the vanilla object.

It literally is the equivalent of applying the setparam ioctl to a fresh
object.

Look at the PXP series for how badly wrong this goes if you try it this
way around.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: remove trailing semicolon in macro definition

[Chris Wilson]

Quoting t...@redhat.com (2020-11-27 16:28:28)
> From: Tom Rix 
> 
> The macro use will already have a semicolon.
> 
> Signed-off-by: Tom Rix 
> ---
>  drivers/gpu/drm/i915/intel_device_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
> b/drivers/gpu/drm/i915/intel_device_info.c
> index e67cec8fa2aa..ef767f04c37c 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -104,7 +104,7 @@ void intel_device_info_print_static(const struct 
> intel_device_info *info,
> drm_printf(p, "ppgtt-type: %d\n", info->ppgtt_type);
> drm_printf(p, "dma_mask_size: %u\n", info->dma_mask_size);
>  
> -#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name));
> +#define PRINT_FLAG(name) drm_printf(p, "%s: %s\n", #name, yesno(info->name))
> DEV_INFO_FOR_EACH_FLAG(PRINT_FLAG);

I thought that this was a macro that avoided adding the ';' to each
invocation. Perhaps another time.

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

Re: [RFC PATCH 001/162] drm/i915/selftest: also consider non-contiguous objects

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:37)
> In igt_ppgtt_sanity_check we should also exercise the non-contiguous
> option for LMEM, since this will give us slightly different sg layouts
> and alignment.
> 
> Signed-off-by: Matthew Auld 
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 004/162] drm/i915/gt: Move move context layout registers and offsets to lrc_reg.h

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:40)
> From: Chris Wilson 
> 
> Cleanup intel_lrc.h by moving some of the residual common register
> definitions into intel_lrc_reg.h, prior to rebranding and splitting off
> the submission backends.
> 
> v2: keep the SCHEDULE enum in the old file, since it is specific to the
> gvt usage of the execlists submission backend (John)
> 
> Signed-off-by: Chris Wilson 
> Signed-off-by: Daniele Ceraolo Spurio  #v2
> Cc: John Harrison 
> Reviewed-by: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  2 +-
>  drivers/gpu/drm/i915/gt/intel_gt_irq.c|  1 +
>  drivers/gpu/drm/i915/gt/intel_lrc.h   | 39 ---
>  drivers/gpu/drm/i915/gt/intel_lrc_reg.h   | 39 +++
>  drivers/gpu/drm/i915/gvt/mmio_context.h   |  2 ++
>  5 files changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index d4e988b2816a..02ea16b29c9f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -36,7 +36,7 @@
>  #include "intel_gt.h"
>  #include "intel_gt_requests.h"
>  #include "intel_gt_pm.h"
> -#include "intel_lrc.h"
> +#include "intel_lrc_reg.h"
>  #include "intel_reset.h"
>  #include "intel_ring.h"
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c 
> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index 257063a57101..9830342aa6f4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -11,6 +11,7 @@
>  #include "intel_breadcrumbs.h"
>  #include "intel_gt.h"
>  #include "intel_gt_irq.h"
> +#include "intel_lrc_reg.h"
>  #include "intel_uncore.h"
>  #include "intel_rps.h"
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h 
> b/drivers/gpu/drm/i915/gt/intel_lrc.h
> index 802585a308e9..9116b46844a2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
> @@ -34,45 +34,6 @@ struct i915_request;
>  struct intel_context;
>  struct intel_engine_cs;
>  
> -/* Execlists regs */
> -#define RING_ELSP(base)_MMIO((base) + 0x230)
> -#define RING_EXECLIST_STATUS_LO(base)  _MMIO((base) + 0x234)
> -#define RING_EXECLIST_STATUS_HI(base)  _MMIO((base) + 0x234 + 4)
> -#define RING_CONTEXT_CONTROL(base) _MMIO((base) + 0x244)
> -#define  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH   (1 << 3)
> -#define  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT   (1 << 0)
> -#define   CTX_CTRL_RS_CTX_ENABLE   (1 << 1)
> -#define  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT  (1 << 2)
> -#define  GEN12_CTX_CTRL_OAR_CONTEXT_ENABLE (1 << 8)
> -#define RING_CONTEXT_STATUS_PTR(base)  _MMIO((base) + 0x3a0)
> -#define RING_EXECLIST_SQ_CONTENTS(base)_MMIO((base) + 0x510)
> -#define RING_EXECLIST_CONTROL(base)_MMIO((base) + 0x550)
> -
> -#define  EL_CTRL_LOAD  (1 << 0)
> -
> -/* The docs specify that the write pointer wraps around after 5h, "After 
> status
> - * is written out to the last available status QW at offset 5h, this pointer
> - * wraps to 0."
> - *
> - * Therefore, one must infer than even though there are 3 bits available, 6 
> and
> - * 7 appear to be * reserved.
> - */
> -#define GEN8_CSB_ENTRIES 6
> -#define GEN8_CSB_PTR_MASK 0x7
> -#define GEN8_CSB_READ_PTR_MASK (GEN8_CSB_PTR_MASK << 8)
> -#define GEN8_CSB_WRITE_PTR_MASK (GEN8_CSB_PTR_MASK << 0)
> -
> -#define GEN11_CSB_ENTRIES 12
> -#define GEN11_CSB_PTR_MASK 0xf
> -#define GEN11_CSB_READ_PTR_MASK (GEN11_CSB_PTR_MASK << 8)
> -#define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
> -
> -#define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
> -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
> -#define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
> -/* in Gen12 ID 0x7FF is reserved to indicate idle */
> -#define GEN12_MAX_CONTEXT_HW_ID(GEN11_MAX_CONTEXT_HW_ID - 1)
> -
>  enum {
> INTEL_CONTEXT_SCHEDULE_IN = 0,
> INTEL_CONTEXT_SCHEDULE_OUT,
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h 
> b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 1b51f7b9a5c3..b2e03ce35599 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -52,4 +52,43 @@
>  #define GEN8_EXECLISTS_STATUS_BUF 0x370
>  #define GEN11_EXECLI

Re: [RFC PATCH 005/162] drm/i915/gt: Rename lrc.c to execlists_submission.c

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:41)
> From: Chris Wilson 
> 
> We want to separate the utility functions for controlling the logical
> ring context from the execlists submission mechanism (which is an
> overgrown scheduler).
> 
> This is similar to Daniele's work to split up the files, but being
> selfish I wanted to base it after my own changes to intel_lrc.c petered
> out.

Note in accordance with recent intel_ring_submission.c vs
intel_ring_scheduler.c, this would be intel_execlists_scheduler.c
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 006/162] drm/i915: split gen8+ flush and bb_start emission functions to their own file

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:04:42)
> From: Daniele Ceraolo Spurio 
> 
> These functions are independent from the backend used and can therefore
> be split out of the exelists submission file, so they can be re-used by
> the upcoming GuC submission backend.
> 
> Based on a patch by Chris Wilson.
> 
> Signed-off-by: Daniele Ceraolo Spurio 
> Cc: Chris P Wilson 
> Cc: Tvrtko Ursulin 
> Reviewed-by: John Harrison 
> ---
>  drivers/gpu/drm/i915/Makefile |   1 +
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.c  | 393 ++
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.h  |  26 ++
>  .../drm/i915/gt/intel_execlists_submission.c  | 385 +
>  4 files changed, 421 insertions(+), 384 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/gen8_engine_cs.c
>  create mode 100644 drivers/gpu/drm/i915/gt/gen8_engine_cs.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index aedbd8f52be8..f9ef5199b124 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,7 @@ gt-y += \
> gt/gen6_engine_cs.o \
> gt/gen6_ppgtt.o \
> gt/gen7_renderclear.o \
> +   gt/gen8_engine_cs.o \
> gt/gen8_ppgtt.o \
> gt/intel_breadcrumbs.o \
> gt/intel_context.o \
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c 
> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> new file mode 100644
> index ..a96fe108685e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -0,0 +1,393 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2014 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_execlists_submission.h" /* XXX */
> +#include "intel_gpu_commands.h"
> +#include "intel_ring.h"
> +
> +int gen8_emit_flush_render(struct i915_request *request, u32 mode)

Refresh the names to make the recent schemes.
(rcs when specific, xcs when not)
-Chris
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Intel-gfx] [PATCH] drm/i915/ehl: Remove require_force_probe protection

[Chris Wilson]

Quoting Pandey, Hariom (2020-10-28 11:55:04)
> Ok, I have initiated the steps to upgrade the CI machine's silicon & BIOS.

The single ehl we have in CI is still failing to enter rc6, both in the
selftest and runtime testing. And I note that RAPL doesn't recognise it,
so it doesn't report the power consumption.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 107/162] drm/i915: setup GPU device lmem region

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:23)
> From: CQ Tang 
> 
> The lmem region needs to remove the stolen part.
> 
> Cc: Joonas Lahtinen 
> Cc: Matthew Auld 
> Cc: Abdiel Janulgue 
> Cc: Chris P Wilson 
> Cc: Balestrieri, Francesco 
> Cc: Niranjana Vishwanathapura 
> Cc: Venkata S Dhanalakota 
> Cc: Neel Desai 
> Cc: Matthew Brost 
> Cc: Sudeep Dutt 
> Signed-off-by: CQ Tang 
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>  drivers/gpu/drm/i915/intel_region_lmem.c | 11 +++
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1af1966ac461..0e01ea0cb0a4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -12066,6 +12066,8 @@ enum skl_power_gate {
>  #define GEN12_LMEM_CFG_ADDR_MMIO(0xcf58)
>  #define   LMEM_ENABLE  (1 << 31)
>  
> +#define GEN12_GSMBASE  _MMIO(0x108100)
> +
>  /* gamt regs */
>  #define GEN8_L3_LRA_1_GPGPU _MMIO(0x4dd4)
>  #define   GEN8_L3_LRA_1_GPGPU_DEFAULT_VALUE_BDW  0x67F1427F /* max/min for 
> LRA1/2 */
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> b/drivers/gpu/drm/i915/intel_region_lmem.c
> index e98582c76de1..7f2b31d469b0 100644
> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -140,20 +140,23 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
>  static struct intel_memory_region *
>  setup_lmem(struct drm_i915_private *dev_priv)

Am I wrong in thinking lmem should be under gt?

>  {
> +   struct intel_uncore *uncore = &dev_priv->uncore;
> struct pci_dev *pdev = dev_priv->drm.pdev;
> struct intel_memory_region *mem;
> resource_size_t io_start;
> -   resource_size_t size;
> +   resource_size_t lmem_size;
>  
> /* Enables Local Memory functionality in GAM */
> I915_WRITE(GEN12_LMEM_CFG_ADDR, I915_READ(GEN12_LMEM_CFG_ADDR) | 
> LMEM_ENABLE);
>  
> +   /* Stolen starts from GSMBASE on DG1 */
> +   lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE);
> +
> io_start = pci_resource_start(pdev, 2);
> -   size = pci_resource_len(pdev, 2);

Sanitycheck the two.

size = min(size, lmem_size);

>  
> mem = intel_memory_region_create(dev_priv,
>  0,
> -size,
> +lmem_size,

Ok, stolen is at tail not start. 

>  I915_GTT_PAGE_SIZE_4K,
>  io_start,
>  &intel_region_lmem_ops);
> @@ -162,7 +165,7 @@ setup_lmem(struct drm_i915_private *dev_priv)
> DRM_INFO("Intel graphics LMEM IO start: %llx\n",
>  (u64)mem->io_start);
> DRM_INFO("Intel graphics LMEM size: %llx\n",
> -(u64)size);
> +(u64)lmem_size);

Use the correct printf-formats, %pa.

> }
>  
> return mem;
> -- 
> 2.26.2
>
-
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 118/162] drm/i915/dg1: Reserve first 1MB of local memory

[Chris Wilson]

Quoting Matthew Auld (2020-11-30 11:09:57)
> On 27/11/2020 13:52, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-11-27 12:06:34)
> >> From: Imre Deak 
> >>
> >> On DG1 A0/B0 steppings the first 1MB of local memory must be reserved.
> >> One reason for this is that the 0xA-0xB range is not accessible
> >> by the display, probably since this region is redirected to another
> >> memory location for legacy VGA compatibility.
> >>
> >> BSpec: 50586
> >> Testcase: igt/kms_big_fb/linear-64bpp-rotate-0
> >> Signed-off-by: Imre Deak 
> >> ---
> >>   drivers/gpu/drm/i915/intel_region_lmem.c | 52 
> >>   1 file changed, 52 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c 
> >> b/drivers/gpu/drm/i915/intel_region_lmem.c
> >> index 939cf0d195a5..eafef7034680 100644
> >> --- a/drivers/gpu/drm/i915/intel_region_lmem.c
> >> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> >> @@ -137,6 +137,48 @@ intel_setup_fake_lmem(struct drm_i915_private *i915)
> >>  return mem;
> >>   }
> >>   
> >> +static void get_legacy_lowmem_region(struct intel_uncore *uncore,
> >> +u64 *start, u32 *size)
> >> +{
> >> +   *start = 0;
> >> +   *size = 0;
> >> +
> >> +   if (!IS_DG1_REVID(uncore->i915, DG1_REVID_A0, DG1_REVID_B0))
> >> +   return;
> >> +
> >> +   *size = SZ_1M;
> >> +
> >> +   DRM_DEBUG_DRIVER("LMEM: reserved legacy low-memory 
> >> [0x%llx-0x%llx]\n",
> >> +*start, *start + *size);
> >> +}
> >> +
> >> +static int reserve_lowmem_region(struct intel_uncore *uncore,
> >> +struct intel_memory_region *mem)
> >> +{
> >> +   u64 reserve_start;
> >> +   u64 reserve_end;
> >> +   u64 region_start;
> >> +   u32 region_size;
> >> +   int ret;
> >> +
> >> +   get_legacy_lowmem_region(uncore, ®ion_start, ®ion_size);
> >> +   reserve_start = region_start;
> >> +   reserve_end = region_start + region_size;
> >> +
> >> +   if (!reserve_end)
> >> +   return 0;
> >> +
> >> +   DRM_INFO("LMEM: reserving low-memory region [0x%llx-0x%llx]\n",
> >> +reserve_start, reserve_end);
> >> +   ret = i915_buddy_alloc_range(&mem->mm, &mem->reserved,
> >> +reserve_start,
> >> +reserve_end - reserve_start);
> > 
> > Isn't this now relative to the stolen offset? Should this be reserved,
> > or excluded like stolen?
> 
> AFAIK stolen is just snipped off at the end of lmem, so I don't think it 
> really matters if we exclude or reserve.

Right, misread, thought it was moving the start point.

> But for this if we exclude then 
> the region.start might have "strange" alignment, which is annoying since 
> alloc(some_power_of_two) might not give us the expected alignment, 
> whereas if we reserve then the allocator is aware, and so we should get 
> the proper alignment. Maybe you have better ideas with how to handle 
> this, but I think keeping the alignment property is nice.

The only tweak I would look at is making this reservation be the
property of the VGA decode. But if this promises not to live into
production, kiss.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 124/162] drm/i915/lmem: allocate HWSP in lmem

[Chris Wilson]

Quoting Matthew Auld (2020-11-30 17:17:16)
> On 27/11/2020 13:55, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-11-27 12:06:40)
> >> From: Michel Thierry 
> > 
> > Rationale goes here.
> > 
> > Is this wise? HWSP is very frequently read by the CPU, and expected to
> > be cached on the CPU.
> > 
> > What do the performance profiles indicate?
> 
> Do you have a recommendation for an existing selftest or IGT to help 
> measure this?
> 
> Also are you suggesting moving this to system memory, or just using a 
> different mapping type, if it's placed in local memory? Or maybe try 
> both? Although I'm pretty sceptical about !wc for local memory.

A lot of worries go out of the window if this can be in system memory
and snooped.

For measuring, I suspect there is a lot of chaff that needs to be
removed before individual microbenchmarks like perf/request discern any
difference; although that would be a starting point. We do a lot of
completion checking during execlists interrupt processing, and there we
(cpu profiles at least) are sensitive to uncached reads.

We can trivially construct a benchmark that only shows the impact of the
WC reads; but the point where I think we would first notice from userspace
is client wakeup latency scaling: benchmarks/gem_latency, which was once
a point of major concern. Nowadays, we can couple that with a second
concern about inducing system latency from interrupt processing time.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:06:08)
> Same old gem_create but with now with extensions support. This is needed
> to support various upcoming usecases. For now we use the extensions
> mechanism to support setting an immutable-priority-list of potential
> placements, at creation time.
> 
> If we wish to set the placements/regions we can simply do:
> 
> struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
> struct drm_i915_gem_create_ext_setparam setparam_region = {
> .base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
> .param = region_param,
> }
> 
> struct drm_i915_gem_create_ext create_ext = {
> .size = 16 * PAGE_SIZE,
> .extensions = (uintptr_t)&setparam_region,
> };
> int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> if (err) ...

Looking at the existing gem_create, there is no detection of an
unsupported extension. That is there is no rejection of new userspace
asking for placement on an old kernel. (As erroneous as that would be
for many other reasons.)

Unless I've missed something, we need a new ioctl number for CREATEv2.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Check the correct variable

[Chris Wilson]

Quoting Dan Carpenter (2020-12-03 08:45:17)
> There is a copy and paste bug in this code.  It's supposed to check
> "obj2" instead of checking "obj" a second time.
> 
> Fixes: 80f0b679d6f0 ("drm/i915: Add an implementation for i915_gem_ww_ctx 
> locking, v2.")
> Signed-off-by: Dan Carpenter 
> ---
>  drivers/gpu/drm/i915/selftests/i915_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c 
> b/drivers/gpu/drm/i915/selftests/i915_gem.c
> index 23a6132c5f4e..412e21604a05 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
> @@ -211,8 +211,8 @@ static int igt_gem_ww_ctx(void *arg)
> return PTR_ERR(obj);
>  
> obj2 = i915_gem_object_create_internal(i915, PAGE_SIZE);
> -   if (IS_ERR(obj)) {
> -   err = PTR_ERR(obj);
> +   if (IS_ERR(obj2)) {
> +   err = PTR_ERR(obj2);

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

Re: [Intel-gfx] [PATCH] drm/i915: Check the correct variable

[Chris Wilson]

Quoting Andi Shyti (2020-12-03 11:12:24)
> Hi Dan,
> 
> > There is a copy and paste bug in this code.  It's supposed to check
> > "obj2" instead of checking "obj" a second time.
> > 
> > Fixes: 80f0b679d6f0 ("drm/i915: Add an implementation for i915_gem_ww_ctx 
> > locking, v2.")
> > Signed-off-by: Dan Carpenter 
> > ---
> >  drivers/gpu/drm/i915/selftests/i915_gem.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c 
> > b/drivers/gpu/drm/i915/selftests/i915_gem.c
> > index 23a6132c5f4e..412e21604a05 100644
> > --- a/drivers/gpu/drm/i915/selftests/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c
> > @@ -211,8 +211,8 @@ static int igt_gem_ww_ctx(void *arg)
> >   return PTR_ERR(obj);
> >  
> >   obj2 = i915_gem_object_create_internal(i915, PAGE_SIZE);
> > - if (IS_ERR(obj)) {
> > - err = PTR_ERR(obj);
> > + if (IS_ERR(obj2)) {
> > + err = PTR_ERR(obj2);
> 
> Reviewed-by: Andi Shyti 

I gave up waiting for patchwork to spot the original patch, and pushed.

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

[PATCH] Revert "drm: convert drm_atomic_uapi.c to new debug helpers"

[Chris Wilson]

Total wipeout in boot!

<7>[3.739908] i915 :00:02.0: 
[drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 0 primary plane
<7>[3.739916] i915 :00:02.0: 
[drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 1 primary plane
9] Hardware name: Hewlett-Packard HP Pro 3500 Series/2ABF, BIOS 8.11 10/24/2012
<4>[3.754904] Workqueue: events_unbound async_run_entry_fn
<4>[3.754908] RIP: 0010:drm_atomic_set_crtc_for_connector+0xe0/0x120
<4>[3.754910] Code: 24 28 be 10 00 00 00 48 c7 c2 60 b0 38 82 48 8b 78 18 
ff 75 20 8b 85 d8 00 00 00 50 e8 89 45 ff ff 58 31 c0 5a 5b 5d 41 5c c3 <48> 8b 
04 25 00 00 00 00 41 8b 4c 24 28 49 89 d9 be 10 00 00 00 4d
<4>[3.754911] RSP: 0018:c92bfa48 EFLAGS: 00010246
<4>[3.754912] RAX: 0005 RBX: 88800ff1a318 RCX: 
0005
<4>[3.754913] RDX: 816d04f0 RSI: 82388e71 RDI: 
88800fc51038
<4>[3.754914] RBP:  R08: 88810414dc10 R09: 
fffe
<4>[3.754915] R10: 682c1dc7 R11: 24f563d5 R12: 
88800fc51000
<4>[3.754916] R13:  R14: 88800ff1a318 R15: 
88801aab9a00
<4>[3.754918] FS:  () GS:88811b48() 
knlGS:
c9/0x130
<4>[3.755084]  do_bind_con_driver+0x1e5/0x2d0
<4>[3.755087]  do_take_over_console+0x10e/0x180
<4>[3.755089]  do_fbcon_takeover+0x53/0xb0
<4>[3.755092]  register_framebuffer+0x22d/0x310
<4>[3.755095]  __drm_fb_helper_initial_config_and_unlock+0x35d/0x530
<4>[3.755190]  intel_fbdev_initial_config+0xf/0x20 [i915]
<4>[3.755192]  async_run_entry_fn+0x34/0x160
<4>[3.755195]  process_one_work+0x270/0x5c0
<4>[3.755199]  worker_thread+0x37/0x380
<4>[3.755201]  ? process_one_work+0x5c0/0x5c0
<4>[3.755203]  kthread+0x146/0x170
<4>[3.755205]  ? kthread_park+0x80/0x80
<4>[3.755208]  ret_from_fork+0x22/0x30
<4>[3.755211] Modules linked in: i915 mei_hdcp x86_pkg_temp_thermal 
coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_intel 
snd_intel_dspcfg snd_hda_codec r8169 snd_hwdep snd_hda_core realtek mei_me 
snd_pcm mei lpc_ich prime_numbers
<4>[3.755224] CR2: 0000
<4>[3.755226] ---[ end trace df071a2078bd01b3 ]---

Fixes: e3aae683e861 ("drm: convert drm_atomic_uapi.c to new debug helpers")
Signed-off-by: Chris Wilson 
Cc: Simon Ser 
Cc: Daniel Vetter 
Cc: Sam Ravnborg 
---
 drivers/gpu/drm/drm_atomic_uapi.c | 113 +-
 1 file changed, 47 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 9df7f2a170e3..d26077ed518a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -85,15 +85,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state 
*state,
 
drm_mode_copy(&state->mode, mode);
state->enable = true;
-   drm_dbg_atomic(crtc->dev,
-  "Set [MODE:%s] for [CRTC:%d:%s] state %p\n",
-  mode->name, crtc->base.id, crtc->name, state);
+   DRM_DEBUG_ATOMIC("Set [MODE:%s] for [CRTC:%d:%s] state %p\n",
+mode->name, crtc->base.id, crtc->name, state);
} else {
memset(&state->mode, 0, sizeof(state->mode));
state->enable = false;
-   drm_dbg_atomic(crtc->dev,
-  "Set [NOMODE] for [CRTC:%d:%s] state %p\n",
-  crtc->base.id, crtc->name, state);
+   DRM_DEBUG_ATOMIC("Set [NOMODE] for [CRTC:%d:%s] state %p\n",
+crtc->base.id, crtc->name, state);
}
 
return 0;
@@ -130,35 +128,31 @@ int drm_atomic_set_mode_prop_for_crtc(struct 
drm_crtc_state *state,
int ret;
 
if (blob->length != sizeof(struct drm_mode_modeinfo)) {
-   drm_dbg_atomic(crtc->dev,
-  "[CRTC:%d:%s] bad mode blob length: 
%zu\n",
-  crtc->base.id, crtc->name,
-  blob->length);
+   DRM_DEBUG_ATOMIC("[CRTC:%d:%s] bad mode blob length: 
%zu\n",
+crtc->base.id, crtc->name,
+blob->length);
return -EINVAL;
}
 
ret = drm_mode_convert_umode(crtc->dev,
 &state->mode, blob->data);
if (ret) {
-

Re: [PATCH] intel: Do not assert on unknown chips in drm_intel_decode_context_alloc

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> From: Tvrtko Ursulin 
> 
> There is this long standing nit of igt/tools/intel_error_decode asserting
> when you feed it an error state from a GPU the local libdrm does not know
> of.
> 
> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> not assert but just return NULL (which seems an already possible return
> value).
> 
> Signed-off-by: Tvrtko Ursulin 

Good riddance,
Reviewed-by: Chris Wilson 
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] intel: Do not assert on unknown chips in drm_intel_decode_context_alloc

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-19 13:42:07)
> 
> On 18/11/2020 17:04, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-11-18 16:36:01)
> >> From: Tvrtko Ursulin 
> >>
> >> There is this long standing nit of igt/tools/intel_error_decode asserting
> >> when you feed it an error state from a GPU the local libdrm does not know
> >> of.
> >>
> >> To fix this I need a tweak in drm_intel_decode_context_alloc to make it
> >> not assert but just return NULL (which seems an already possible return
> >> value).
> >>
> >> Signed-off-by: Tvrtko Ursulin 
> > 
> > Good riddance,
> > Reviewed-by: Chris Wilson 
> 
> Thanks, now how can push to drm and is there some testing to be 
> triggered before, or after?

cd intel; for i in tests/gen*.sh; do $i; done

But clearly I haven't built libdrm since automake was dropped.
-Chris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/2] chrontel-ch7033: Add byteswap order option

[Chris Morgan]

From: Chris Morgan 

This series adds the ability to set the byteswap order in the chrontel
ch7033 driver via an optional devicetree node. This is necessary
because the HDMI DIP of the NTC CHIP requires a byteswap order that
differs from the default value of the driver.

Signed-off-by: Chris Morgan 

Chris Morgan (2):
  dt-bindings: Add byteswap order to chrontel ch7033
  drm/bridge: chrontel-ch7033: Add option for setting byteswap order

 .../bindings/display/bridge/chrontel,ch7033.yaml  |  8 
 drivers/gpu/drm/bridge/chrontel-ch7033.c  | 15 +--
 2 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] dt-bindings: Add byteswap order to chrontel ch7033

[Chris Morgan]

From: Chris Morgan 

Update dt-binding documentation to add support for setting byteswap of
chrontel ch7033.

New property name of chrontel,byteswap added to set the byteswap order.
This property is optional.

Signed-off-by: Chris Morgan 
---
 .../bindings/display/bridge/chrontel,ch7033.yaml  | 8 
 1 file changed, 8 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml 
b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
index bb6289c7d375..ecd3062c5215 100644
--- a/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/chrontel,ch7033.yaml
@@ -14,6 +14,14 @@ properties:
   compatible:
 const: chrontel,ch7033
 
+  chrontel,byteswap:
+$ref: /schemas/types.yaml#/definitions/uint8
+enum: [0, 1, 2, 3, 4, 5]
+description: |
+  Set the byteswap value of the bridge. Values 0-5 correspond to
+  BYTE_SWAP_RGB, BYTE_SWAP_RBG, BYTE_SWAP_GRB, BYTE_SWAP_GBR,
+  BYTE_SWAP_BRG, and BYTE_SWAP_BGR respectively.
+
   reg:
 maxItems: 1
 description: I2C address of the device
-- 
2.25.1