[PATCHv3] modesetting: Update fb_id from shadow allocate and destroy if not set

Submitted by Tony Lindgren on July 5, 2018, 9:27 a.m.

Details

Message ID 20180705092705.61627-1-tony@atomide.com
State New
Series "modesetting: Update fb_id from shadow allocate and destroy if not set"
Headers show

Commit Message

Tony Lindgren July 5, 2018, 9:27 a.m.
Looks like drmModeDirtyFB() stopped working at some point and we now
call it with fb_id of zero for rotated displays. This will stop displays
relying on DRM_IOCTL_MODE_DIRTYFB ioctl to display anything.

This regression probably with commit 3dcd591fa9b7 ("modesetting: Add
support for using RandR shadow buffers") that inroduced rotate_fb_id.

Let's fix this issue by going through all the displays.

Cc: Hans De Goede <hdegoede@redhat.com>
Cc: Jason Ekstrand <jason.ekstrand@intel.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Lyude Paul <lyude@redhat.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Here's this one resent with proper patch description, sorry
for the delays sending it out.

---

 hw/xfree86/drivers/modesetting/driver.c | 41 +++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
--- a/hw/xfree86/drivers/modesetting/driver.c
+++ b/hw/xfree86/drivers/modesetting/driver.c
@@ -531,12 +531,11 @@  dispatch_dirty_region(ScrnInfoPtr scrn,
 }
 
 static void
-dispatch_dirty(ScreenPtr pScreen)
+dispatch_dirty_fb_id(ScreenPtr pScreen, int fb_id)
 {
     ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
     modesettingPtr ms = modesettingPTR(scrn);
     PixmapPtr pixmap = pScreen->GetScreenPixmap(pScreen);
-    int fb_id = ms->drmmode.fb_id;
     int ret;
 
     ret = dispatch_dirty_region(scrn, pixmap, ms->damage, fb_id);
@@ -547,7 +546,43 @@  dispatch_dirty(ScreenPtr pScreen)
         ms->damage = NULL;
         xf86DrvMsg(scrn->scrnIndex, X_INFO,
                    "Disabling kernel dirty updates, not required.\n");
-        return;
+    }
+}
+
+static void
+dispatch_dirty(ScreenPtr pScreen)
+{
+    ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
+    modesettingPtr ms = modesettingPTR(scrn);
+    modesettingEntPtr ms_ent = ms_ent_priv(scrn);
+    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
+    xf86CrtcPtr crtc;
+    drmmode_crtc_private_ptr drmmode_crtc;
+    unsigned int mask;
+
+    mask = ms_ent->assigned_crtcs;
+
+    while (mask) {
+        int i, fb_id = 0;
+
+        i = ffs(mask) - 1;
+
+        crtc = xf86_config->crtc[i];
+        if (!ms_crtc_on(crtc))
+            goto skip;
+
+        drmmode_crtc = crtc->driver_private;
+
+        if (drmmode_crtc->rotate_fb_id)
+            fb_id = drmmode_crtc->rotate_fb_id;
+        else
+            fb_id = ms->drmmode.fb_id;
+
+        if (fb_id)
+            dispatch_dirty_fb_id(pScreen, fb_id);
+
+    skip:
+        mask &= ~(1 << i);
     }
 }
 

Comments

Lyude Paul July 5, 2018, 1:42 p.m.
Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-07-05 at 02:27 -0700, Tony Lindgren wrote:
> Looks like drmModeDirtyFB() stopped working at some point and we now
> call it with fb_id of zero for rotated displays. This will stop displays
> relying on DRM_IOCTL_MODE_DIRTYFB ioctl to display anything.
> 
> This regression probably with commit 3dcd591fa9b7 ("modesetting: Add
> support for using RandR shadow buffers") that inroduced rotate_fb_id.
> 
> Let's fix this issue by going through all the displays.
> 
> Cc: Hans De Goede <hdegoede@redhat.com>
> Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Here's this one resent with proper patch description, sorry
> for the delays sending it out.
> 
> ---
> 
>  hw/xfree86/drivers/modesetting/driver.c | 41 +++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/driver.c
> b/hw/xfree86/drivers/modesetting/driver.c
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -531,12 +531,11 @@ dispatch_dirty_region(ScrnInfoPtr scrn,
>  }
>  
>  static void
> -dispatch_dirty(ScreenPtr pScreen)
> +dispatch_dirty_fb_id(ScreenPtr pScreen, int fb_id)
>  {
>      ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
>      modesettingPtr ms = modesettingPTR(scrn);
>      PixmapPtr pixmap = pScreen->GetScreenPixmap(pScreen);
> -    int fb_id = ms->drmmode.fb_id;
>      int ret;
>  
>      ret = dispatch_dirty_region(scrn, pixmap, ms->damage, fb_id);
> @@ -547,7 +546,43 @@ dispatch_dirty(ScreenPtr pScreen)
>          ms->damage = NULL;
>          xf86DrvMsg(scrn->scrnIndex, X_INFO,
>                     "Disabling kernel dirty updates, not required.\n");
> -        return;
> +    }
> +}
> +
> +static void
> +dispatch_dirty(ScreenPtr pScreen)
> +{
> +    ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
> +    modesettingPtr ms = modesettingPTR(scrn);
> +    modesettingEntPtr ms_ent = ms_ent_priv(scrn);
> +    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
> +    xf86CrtcPtr crtc;
> +    drmmode_crtc_private_ptr drmmode_crtc;
> +    unsigned int mask;
> +
> +    mask = ms_ent->assigned_crtcs;
> +
> +    while (mask) {
> +        int i, fb_id = 0;
> +
> +        i = ffs(mask) - 1;
> +
> +        crtc = xf86_config->crtc[i];
> +        if (!ms_crtc_on(crtc))
> +            goto skip;
> +
> +        drmmode_crtc = crtc->driver_private;
> +
> +        if (drmmode_crtc->rotate_fb_id)
> +            fb_id = drmmode_crtc->rotate_fb_id;
> +        else
> +            fb_id = ms->drmmode.fb_id;
> +
> +        if (fb_id)
> +            dispatch_dirty_fb_id(pScreen, fb_id);
> +
> +    skip:
> +        mask &= ~(1 << i);
>      }
>  }
>
Michel Dänzer July 5, 2018, 2:18 p.m.
On 2018-07-05 11:27 AM, Tony Lindgren wrote:
> Looks like drmModeDirtyFB() stopped working at some point and we now
> call it with fb_id of zero for rotated displays. This will stop displays
> relying on DRM_IOCTL_MODE_DIRTYFB ioctl to display anything.
> 
> This regression probably with commit 3dcd591fa9b7 ("modesetting: Add
> support for using RandR shadow buffers") that inroduced rotate_fb_id.
> 
> Let's fix this issue by going through all the displays.
> 
> Cc: Hans De Goede <hdegoede@redhat.com>
> Cc: Jason Ekstrand <jason.ekstrand@intel.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Here's this one resent with proper patch description, sorry
> for the delays sending it out.
> 
> ---
> 
>  hw/xfree86/drivers/modesetting/driver.c | 41 +++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -531,12 +531,11 @@ dispatch_dirty_region(ScrnInfoPtr scrn,
>  }
>  
>  static void
> -dispatch_dirty(ScreenPtr pScreen)
> +dispatch_dirty_fb_id(ScreenPtr pScreen, int fb_id)
>  {
>      ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
>      modesettingPtr ms = modesettingPTR(scrn);
>      PixmapPtr pixmap = pScreen->GetScreenPixmap(pScreen);
> -    int fb_id = ms->drmmode.fb_id;
>      int ret;
>  
>      ret = dispatch_dirty_region(scrn, pixmap, ms->damage, fb_id);
> @@ -547,7 +546,43 @@ dispatch_dirty(ScreenPtr pScreen)
>          ms->damage = NULL;
>          xf86DrvMsg(scrn->scrnIndex, X_INFO,
>                     "Disabling kernel dirty updates, not required.\n");
> -        return;
> +    }
> +}
> +
> +static void
> +dispatch_dirty(ScreenPtr pScreen)
> +{
> +    ScrnInfoPtr scrn = xf86ScreenToScrn(pScreen);
> +    modesettingPtr ms = modesettingPTR(scrn);
> +    modesettingEntPtr ms_ent = ms_ent_priv(scrn);
> +    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
> +    xf86CrtcPtr crtc;
> +    drmmode_crtc_private_ptr drmmode_crtc;
> +    unsigned int mask;
> +
> +    mask = ms_ent->assigned_crtcs;
> +
> +    while (mask) {
> +        int i, fb_id = 0;
> +
> +        i = ffs(mask) - 1;
> +
> +        crtc = xf86_config->crtc[i];
> +        if (!ms_crtc_on(crtc))
> +            goto skip;
> +
> +        drmmode_crtc = crtc->driver_private;
> +
> +        if (drmmode_crtc->rotate_fb_id)
> +            fb_id = drmmode_crtc->rotate_fb_id;
> +        else
> +            fb_id = ms->drmmode.fb_id;
> +
> +        if (fb_id)
> +            dispatch_dirty_fb_id(pScreen, fb_id);
> +
> +    skip:
> +        mask &= ~(1 << i);
>      }
>  }
>  
> 

This uses the same damage region for all framebuffers, which is
generally not correct for drmmode_crtc->rotate_fb_id. The coordinate
offset, rotation and reflection need to be taken into account for that.
Tony Lindgren July 5, 2018, 3:42 p.m.
Hi,

* Michel Dänzer <michel@daenzer.net> [180705 14:21]:
> 
> This uses the same damage region for all framebuffers, which is
> generally not correct for drmmode_crtc->rotate_fb_id. The coordinate
> offset, rotation and reflection need to be taken into account for that.

Hmm OK. I thought we just need to refresh it we have
rotate_fb_id.

Unfortunately I have no idea what the check here might be
for the variables above.. Sounds like it might leave out
some pointless updates though :) Care to describe a bit
more or ideally even provide a patch to test?

Regards,

Tony
Michel Dänzer July 6, 2018, 7:45 a.m.
On 2018-07-05 05:42 PM, Tony Lindgren wrote:
> Hi,
> 
> * Michel Dänzer <michel@daenzer.net> [180705 14:21]:
>>
>> This uses the same damage region for all framebuffers, which is
>> generally not correct for drmmode_crtc->rotate_fb_id. The coordinate
>> offset, rotation and reflection need to be taken into account for that.
> 
> Hmm OK. I thought we just need to refresh it we have
> rotate_fb_id.
> 
> Unfortunately I have no idea what the check here might be
> for the variables above.. Sounds like it might leave out
> some pointless updates though :) Care to describe a bit
> more or ideally even provide a patch to test?

The simplest solution is probably to use separate damage records
attached to the per-CRTC rotation pixmaps.
Tony Lindgren July 6, 2018, 10:41 a.m.
* Michel Dänzer <michel@daenzer.net> [180706 07:48]:
> On 2018-07-05 05:42 PM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Michel Dänzer <michel@daenzer.net> [180705 14:21]:
> >>
> >> This uses the same damage region for all framebuffers, which is
> >> generally not correct for drmmode_crtc->rotate_fb_id. The coordinate
> >> offset, rotation and reflection need to be taken into account for that.
> > 
> > Hmm OK. I thought we just need to refresh it we have
> > rotate_fb_id.
> > 
> > Unfortunately I have no idea what the check here might be
> > for the variables above.. Sounds like it might leave out
> > some pointless updates though :) Care to describe a bit
> > more or ideally even provide a patch to test?
> 
> The simplest solution is probably to use separate damage records
> attached to the per-CRTC rotation pixmaps.

Sorry but I still have no idea what needs to be checked
with rotate_fb_id and rotation pixmaps before updating
the display..

Regards,

Tony