diff mbox series

[1/6] drm/msm: Clear aperture ownership outside of fbdev code

Message ID 20230330074150.7637-2-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series drm/msm: Convert fbdev to DRM client | expand

Commit Message

Thomas Zimmermann March 30, 2023, 7:41 a.m. UTC
Move aperture management out of the fbdev code. It is unrelated
and needs to run even if fbdev support has been disabled. Call
the helper at the top of msm_drm_init() to take over hardware
from other drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/msm/msm_drv.c   | 6 ++++++
 drivers/gpu/drm/msm/msm_fbdev.c | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Dmitry Baryshkov March 30, 2023, 9:51 a.m. UTC | #1
On Thu, 30 Mar 2023 at 10:41, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Move aperture management out of the fbdev code. It is unrelated
> and needs to run even if fbdev support has been disabled. Call
> the helper at the top of msm_drm_init() to take over hardware
> from other drivers.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/msm/msm_drv.c   | 6 ++++++
>  drivers/gpu/drm/msm/msm_fbdev.c | 6 ------
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index aca48c868c14..5211140ec50b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -12,6 +12,7 @@
>  #include <linux/uaccess.h>
>  #include <uapi/linux/sched/types.h>
>
> +#include <drm/drm_aperture.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
> @@ -411,6 +412,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>         if (drm_firmware_drivers_only())
>                 return -ENODEV;
>
> +       /* the fw fb could be anywhere in memory */
> +       ret = drm_aperture_remove_framebuffers(false, drv);
> +       if (ret)
> +               return ret;
> +

I think it is not a good place to remove framebuffers. EFIFB might be
still alive and if we kick it out, it might be very hard to debug what
went wrong.
Could you please move it after component bind? Then we can be sure at
least that all subdevices are bound. I see that armada and sun4i call
it as late as possible, when no calls can fail.

>         ddev = drm_dev_alloc(drv, dev);
>         if (IS_ERR(ddev)) {
>                 DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index d26aa52217ce..fc7d0406a9f9 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -4,7 +4,6 @@
>   * Author: Rob Clark <robdclark@gmail.com>
>   */
>
> -#include <drm/drm_aperture.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_fourcc.h>
> @@ -154,11 +153,6 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
>                 goto fail;
>         }
>
> -       /* the fw fb could be anywhere in memory */
> -       ret = drm_aperture_remove_framebuffers(false, dev->driver);
> -       if (ret)
> -               goto fini;
> -
>         ret = drm_fb_helper_initial_config(helper);
>         if (ret)
>                 goto fini;
> --
> 2.40.0
>
Thomas Zimmermann April 3, 2023, 11:19 a.m. UTC | #2
Hi

Am 30.03.23 um 11:51 schrieb Dmitry Baryshkov:
> On Thu, 30 Mar 2023 at 10:41, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Move aperture management out of the fbdev code. It is unrelated
>> and needs to run even if fbdev support has been disabled. Call
>> the helper at the top of msm_drm_init() to take over hardware
>> from other drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/msm/msm_drv.c   | 6 ++++++
>>   drivers/gpu/drm/msm/msm_fbdev.c | 6 ------
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index aca48c868c14..5211140ec50b 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/uaccess.h>
>>   #include <uapi/linux/sched/types.h>
>>
>> +#include <drm/drm_aperture.h>
>>   #include <drm/drm_bridge.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_file.h>
>> @@ -411,6 +412,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
>>          if (drm_firmware_drivers_only())
>>                  return -ENODEV;
>>
>> +       /* the fw fb could be anywhere in memory */
>> +       ret = drm_aperture_remove_framebuffers(false, drv);
>> +       if (ret)
>> +               return ret;
>> +
> 
> I think it is not a good place to remove framebuffers. EFIFB might be
> still alive and if we kick it out, it might be very hard to debug what
> went wrong.
> Could you please move it after component bind? Then we can be sure at
> least that all subdevices are bound. I see that armada and sun4i call
> it as late as possible, when no calls can fail.

Ok. I briefly looked over the code to make sure that no code touches HW 
settings before msm acquires the device.

Best regards
Thomas

> 
>>          ddev = drm_dev_alloc(drv, dev);
>>          if (IS_ERR(ddev)) {
>>                  DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
>> index d26aa52217ce..fc7d0406a9f9 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -4,7 +4,6 @@
>>    * Author: Rob Clark <robdclark@gmail.com>
>>    */
>>
>> -#include <drm/drm_aperture.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_fb_helper.h>
>>   #include <drm/drm_fourcc.h>
>> @@ -154,11 +153,6 @@ struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
>>                  goto fail;
>>          }
>>
>> -       /* the fw fb could be anywhere in memory */
>> -       ret = drm_aperture_remove_framebuffers(false, dev->driver);
>> -       if (ret)
>> -               goto fini;
>> -
>>          ret = drm_fb_helper_initial_config(helper);
>>          if (ret)
>>                  goto fini;
>> --
>> 2.40.0
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index aca48c868c14..5211140ec50b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -12,6 +12,7 @@ 
 #include <linux/uaccess.h>
 #include <uapi/linux/sched/types.h>
 
+#include <drm/drm_aperture.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
@@ -411,6 +412,11 @@  static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	if (drm_firmware_drivers_only())
 		return -ENODEV;
 
+	/* the fw fb could be anywhere in memory */
+	ret = drm_aperture_remove_framebuffers(false, drv);
+	if (ret)
+		return ret;
+
 	ddev = drm_dev_alloc(drv, dev);
 	if (IS_ERR(ddev)) {
 		DRM_DEV_ERROR(dev, "failed to allocate drm_device\n");
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index d26aa52217ce..fc7d0406a9f9 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -4,7 +4,6 @@ 
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
-#include <drm/drm_aperture.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
@@ -154,11 +153,6 @@  struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	/* the fw fb could be anywhere in memory */
-	ret = drm_aperture_remove_framebuffers(false, dev->driver);
-	if (ret)
-		goto fini;
-
 	ret = drm_fb_helper_initial_config(helper);
 	if (ret)
 		goto fini;