Re: [PATCH v2] drm/msm/adreno: Assign revn to A635

2023-07-01 Thread Dmitry Baryshkov 
On Sat, 1 Jul 2023 at 18:50, Rob Clark  wrote:
>
> On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio  
> wrote:
> >
> > Recently, a WARN_ON() was introduced to ensure that revn is filled before
> > adreno_is_aXYZ is called. This however doesn't work very well when revn is
> > 0 by design (such as for A635). Fill it in as a stopgap solution for
> > -fixes.
> >
> > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before 
> > being set")
> > Signed-off-by: Konrad Dybcio 
> > ---
> > Changes in v2:
> > - add fixes
> > - Link to v1: 
> > https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c0...@linaro.org
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index cb94cfd137a8..8ea7eae9fc52 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = {
> > .address_space_size = SZ_16G,
> > }, {
> > .rev = ADRENO_REV(6, 3, 5, ANY_ID),
> > +   .revn = 635,
> > .fw = {
> > [ADRENO_FW_SQE] = "a660_sqe.fw",
> > [ADRENO_FW_GMU] = "a660_gmu.bin",
> >
>
> hmm, I realized a problem with this, it would change what
> MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return..  The
> former should be "harmless", although it isn't a good idea for uabi
> changes to be a side effect of a fix.  The latter is more problematic.

I'd say MSM_PARAM_GPU_ID is broken for 635 anyway (won't it return 0
in this case)?
So the new value should be correct.

But more importantly, why are we exporting speedbin in
MSM_PARAM_CHIP_ID only if there is no revn? And why are we exporting
the speedbin at all as a part of CHIP_ID?

>
> I think I'm leaning more towards reverting commit cc943f43ece7
> ("drm/msm/adreno: warn if chip revn is verified before being set") for
> -fixes.  I'm still thinking about options for a longer term fix.
>
> BR,
> -R
>
>
> > ---
> > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2
> > change-id: 20230628-topic-a635-1b3c2c987417
> >
> > Best regards,
> > --
> > Konrad Dybcio 
> >



-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/adreno: warn if GPU revision is not known yet

2023-07-01 Thread Rob Clark 
On Sat, Jul 1, 2023 at 10:15 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
> 510") tried to check GPU's revn before revn being set. Add WARN_ON_ONCE
> to prevent such bugs from happening again. A separate helper is
> necessary so that the warning is displayed really just once instead of
> being displayed for each of comparisons.
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h | 70 +++--
>  1 file changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 506001080374..eb31c83582e6 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -145,143 +145,155 @@ struct adreno_platform_config {
>
>  bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>
> +static inline const struct adreno_info
> +gpu_info(struct adreno_gpu *gpu)
> +{
> +   if (WARN_ON_ONCE(!gpu->info))
> +   return (struct adreno_info) {};
> +   return *gpu->info;
> +}
> +
>  static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
>  {
> -   return (gpu->revn < 300);
> +   return (gpu_info(gpu).revn < 300);
>  }
>
>  static inline bool adreno_is_a20x(struct adreno_gpu *gpu)
>  {
> -   return (gpu->revn < 210);
> +   return (gpu_info(gpu).revn < 210);
>  }
>
>  static inline bool adreno_is_a225(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 225;
> +   return gpu_info(gpu).revn == 225;
>  }
>
>  static inline bool adreno_is_a305(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 305;
> +   return gpu_info(gpu).revn == 305;
>  }
>
>  static inline bool adreno_is_a306(struct adreno_gpu *gpu)
>  {
> /* yes, 307, because a305c is 306 */
> -   return gpu->revn == 307;
> +   return gpu_info(gpu).revn == 307;
>  }
>
>  static inline bool adreno_is_a320(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 320;
> +   return gpu_info(gpu).revn == 320;
>  }
>
>  static inline bool adreno_is_a330(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 330;
> +   return gpu_info(gpu).revn == 330;
>  }
>
>  static inline bool adreno_is_a330v2(struct adreno_gpu *gpu)
>  {
> -   return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
> +   return adreno_is_a330(gpu) && (gpu_info(gpu).rev.patchid > 0);

This still isn't quite right, because adreno_info::rev has wildcards..
but I think the basic idea of checking that gpu->info is already set
is what we want..

I'll might punt on this until a bigger re-work of device
identification, since at least for now the root issue is solved

BR,
-R

>  }
>
>  static inline int adreno_is_a405(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 405;
> +   return gpu_info(gpu).revn == 405;
>  }
>
>  static inline int adreno_is_a420(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 420;
> +   return gpu_info(gpu).revn == 420;
>  }
>
>  static inline int adreno_is_a430(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 430;
> +   return gpu_info(gpu).revn == 430;
>  }
>
>  static inline int adreno_is_a506(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 506;
> +   return gpu_info(gpu).revn == 506;
>  }
>
>  static inline int adreno_is_a508(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 508;
> +   return gpu_info(gpu).revn == 508;
>  }
>
>  static inline int adreno_is_a509(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 509;
> +   return gpu_info(gpu).revn == 509;
>  }
>
>  static inline int adreno_is_a510(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 510;
> +   return gpu_info(gpu).revn == 510;
>  }
>
>  static inline int adreno_is_a512(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 512;
> +   return gpu_info(gpu).revn == 512;
>  }
>
>  static inline int adreno_is_a530(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 530;
> +   return gpu_info(gpu).revn == 530;
>  }
>
>  static inline int adreno_is_a540(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 540;
> +   return gpu_info(gpu).revn == 540;
>  }
>
>  static inline int adreno_is_a618(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 618;
> +   return gpu_info(gpu).revn == 618;
>  }
>
>  static inline int adreno_is_a619(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 619;
> +   return gpu_info(gpu).revn == 619;
>  }
>
>  static inline int adreno_is_a630(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 630;
> +   return gpu_info(gpu).revn == 630;
>  }
>
>  static inline int adreno_is_a640_family(struct adreno_gpu *gpu)
>  {
> -   return (gpu->revn == 640) || (gpu->revn == 680);
> +   return (gpu_info(gpu).revn == 640) ||
> +   (gpu_info(gpu).revn == 680);
>  }
>
>  static inline int adreno_is_a65

Re: [PATCH v2 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols

2023-07-01 Thread Arnd Bergmann 
On Sat, Jul 1, 2023, at 23:44, Javier Martinez Canillas wrote:
> Currently the CONFIG_FB option has to be enabled even if no legacy fbdev
> drivers are needed (e.g: only to have support for framebuffer consoles).
>
> The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB
> and so it can only be enabled if that dependency is enabled as well.
>
> That means fbdev drivers have to be explicitly disabled if users want to
> enable CONFIG_FB, only to use fbcon and/or the DRM fbdev emulation layer.
>
> This patch introduces a non-visible CONFIG_FB_CORE symbol that could be
> enabled just to have core support needed for CONFIG_DRM_FBDEV_EMULATION,
> allowing CONFIG_FB to be disabled (and automatically disabling all the
> fbdev drivers).
>
> Signed-off-by: Javier Martinez Canillas 
> ---

I found two more things now:

> 
> +menuconfig FB_CORE
> + tristate "Core support for frame buffer devices"
> +

This is not actually a hidden option, since you left the prompt
after the 'tristate' keyword. There is also no pointn in having
it as a menu, just use the simpler

config FB_CORE
tristate

or (as in my other email)

config FB_CORE
   def_tristate FB || (DRM && DRM_FBDEV_EMULATION)


> @@ -44,7 +54,7 @@ menuconfig FB
> 
>  config FIRMWARE_EDID
>   bool "Enable firmware EDID"
> - depends on FB
> + depends on FB_CORE
>   help
> This enables access to the EDID transferred from the firmware.
> On the i386, this is from the Video BIOS. Enable this if DDC/I2C
> @@ -59,7 +69,7 @@ config FIRMWARE_EDID
> 
>  config FB_DEVICE
>   bool "Provide legacy /dev/fb* device"
> - depends on FB
> + select FB_CORE
>   default y
>   help
> Say Y here if you want the legacy /dev/fb* device file and

These are now the only user visible sub-options when CONFIG_FB is
disabled. I missed FIRMWARE_EDID earlier, but this also looks like
it can clearly be left as depending on FB since nothing else calls
fb_firmware_edid. In fact, it looks like all of fbmon.c could be
left out since none of its exported symbols are needed for DRM.

That would leave CONFIG_FB_DEVICE as the only user visible option
for DRM-only configs, which is slightly odd for the menuconfig,
so I still wonder if that could be done differently.

Is there actually a point in configurations for kernels with FB=y,
DRM=n and FB_DEVICE=n? If we don't expect that to be a useful
configuration, an easier way would be to have CONFIG_FB turn it
on implicitly and instead have a user-visible Kconfig option
below CONFIG_DRM_FBDEV_EMULATION that allows controlling the
creation of /dev/fb*.

 Arnd

Re: [PATCH v2 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols

2023-07-01 Thread Randy Dunlap 
Hi,


Does this series apply on top of the previous series or on what?


On 7/1/23 14:44, Javier Martinez Canillas wrote:
> Currently the CONFIG_FB option has to be enabled even if no legacy fbdev
> drivers are needed (e.g: only to have support for framebuffer consoles).
> 
> The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB
> and so it can only be enabled if that dependency is enabled as well.
> 
> That means fbdev drivers have to be explicitly disabled if users want to
> enable CONFIG_FB, only to use fbcon and/or the DRM fbdev emulation layer.
> 
> This patch introduces a non-visible CONFIG_FB_CORE symbol that could be
> enabled just to have core support needed for CONFIG_DRM_FBDEV_EMULATION,
> allowing CONFIG_FB to be disabled (and automatically disabling all the
> fbdev drivers).
> 
> Signed-off-by: Javier Martinez Canillas 
> ---
> 
> Changes in v2:
> - Keep "depends on FB" for FB_DDC, FB_HECUBA, FB_SVGALIB, FB_MACMODES,
>   FB_BACKLIGHT, FB_MODE_HELPERS and FB_TILEBLITTING (Arnd Bergmann).
> - Don't change the fb.o object name (Arnd Bergmann).
> - Make FB_CORE a non-visible Kconfig symbol instead (Thomas Zimmermann).
> 
>  arch/x86/Makefile |  2 +-
>  arch/x86/video/Makefile   |  2 +-
>  drivers/video/console/Kconfig |  2 +-
>  drivers/video/fbdev/Kconfig   | 40 +++
>  drivers/video/fbdev/core/Makefile |  2 +-
>  5 files changed, 29 insertions(+), 19 deletions(-)
> 

> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index cecf15418632..da6f7d588f17 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -6,8 +6,12 @@
>  config FB_NOTIFY
>   bool
>  
> +menuconfig FB_CORE
> + tristate "Core support for frame buffer devices"
> +

I could be reading this incorrectly, but FB_CORE does not appear to
be a non-visible Kconfig symbol here.

>  menuconfig FB
> - tristate "Support for frame buffer devices"
> + tristate "Support for frame buffer device drivers"
> + select FB_CORE
>   select FB_NOTIFY
>   select VIDEO_CMDLINE
>   help

thanks.
-- 
~Randy

Re: [PATCH v2 2/2] drm: Make fbdev emulation select FB_CORE instead of depends on FB

2023-07-01 Thread Arnd Bergmann 
On Sat, Jul 1, 2023, at 23:44, Javier Martinez Canillas wrote:
> Now that the fbdev core has been split in FB_CORE and FB, make DRM fbdev
> emulation layer to just select the former.
>
> This allows to disable the CONFIG_FB option if is not needed, which will
> avoid the need to explicitly disable each of the legacy fbdev drivers.
>
> Signed-off-by: Javier Martinez Canillas 
> ---
>
> Changes in v2:
> - Make CONFIG_DRM_FBDEV_EMULATION to select FB_CORE (Thomas Zimmermann).
>
>  drivers/gpu/drm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index afb3b2f5f425..d9b1710e3ad0 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -132,7 +132,7 @@ config DRM_DEBUG_MODESET_LOCK
>  config DRM_FBDEV_EMULATION
>   bool "Enable legacy fbdev support for your modesetting driver"
>   depends on DRM_KMS_HELPER
> - depends on FB=y || FB=DRM_KMS_HELPER
> + select FB_CORE

This will unfortunately force FB_CORE=y even with DRM=m, it would be nice
to allow both to be loadable modules. Any of these should work:

a) Add another hidden symbol like

config DRM_FB_CORE
  def_tristate DRM && DRM_FBDEV_EMULATION
  select FB_CORE

b) move the 'select' to DRM

config DRM
  tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
  select FB_CORE if DRM_FBDEV_EMULATION

c) Remove the 'select' and instead use the default 

config FB_CORE
 def_tristate FB || (DRM && DRM_FBDEV_EMULATION)

   Arnd

Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support

2023-07-01 Thread Sam Ravnborg 
Hi Julius.

Thanks for posting this. I few nits in the following where you as the
author decide what to ignore and what to update.

Sam


On Sat, Jul 01, 2023 at 02:08:03PM +0200, Julius Zint wrote:
> The Apple Studio Display does not have any physical buttons and the only
> way to get or set the brightness is by sending USB control transfers to a
> HID device exposed by the display.
> 
> These control transfers take the form of a HID_(GET|SET)_REPORT request
> and the payload looks like this:
> 
> struct brightness_ctrl_message_data {
>u8 unknown_1;
>__le16 brightness;
>u8 unkown_2[4];
> } __packed;
> 
> When compiled as a module this driver needs to be part of the early boot
> environment, otherwise the generic USB HID driver will claim the device.

I hope someone else can help here, as I have no clue.

> 
> Signed-off-by: Julius Zint 
> ---
>  drivers/video/backlight/Kconfig|   8 +
>  drivers/video/backlight/Makefile   |   1 +
>  drivers/video/backlight/apple_bl_usb.c | 264 +
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/video/backlight/apple_bl_usb.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..9383d402ebed 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -290,6 +290,14 @@ config BACKLIGHT_APPLE
> If you have an Intel-based Apple say Y to enable a driver for its
> backlight.
>  
> +config BACKLIGHT_APPLE_USB
> + tristate "Apple USB Backlight Driver"
> + depends on USB
> + help
> +   If you have an external display from Apple that is attached via USB
> +   say Y to enable a driver for its backlight. Currently it supports the
> +   Apple Studio Display.
> +
>  config BACKLIGHT_QCOM_WLED
>   tristate "Qualcomm PMIC WLED Driver"
>   select REGMAP
> diff --git a/drivers/video/backlight/Makefile 
> b/drivers/video/backlight/Makefile
> index f72e1c3c59e9..c42880655113 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520) += adp5520_bl.o
>  obj-$(CONFIG_BACKLIGHT_ADP8860)  += adp8860_bl.o
>  obj-$(CONFIG_BACKLIGHT_ADP8870)  += adp8870_bl.o
>  obj-$(CONFIG_BACKLIGHT_APPLE)+= apple_bl.o
> +obj-$(CONFIG_BACKLIGHT_APPLE_USB)+= apple_bl_usb.o
>  obj-$(CONFIG_BACKLIGHT_AS3711)   += as3711_bl.o
>  obj-$(CONFIG_BACKLIGHT_BD6107)   += bd6107.o
>  obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH)+= cr_bllcd.o
> diff --git a/drivers/video/backlight/apple_bl_usb.c 
> b/drivers/video/backlight/apple_bl_usb.c
> new file mode 100644
> index ..b746b7822974
> --- /dev/null
> +++ b/drivers/video/backlight/apple_bl_usb.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
> +
> +#define HID_GET_REPORT 0x01
> +#define HID_SET_REPORT 0x09
> +
> +#define HID_REPORT_TYPE_FEATURE 0x0300
> +
> +struct apple_bl_usb_data {
> + struct usb_interface *usb_interface;
> + struct usb_device *usb_dev;
> +};
> +
> +struct brightness_ctrl_message_data {
> + u8 unknown_1;
> + __le16 brightness;
> + u8 unkown_2[4];
> +} __packed;

A different way to set the brightness value could be:
struct brightness_ctrl_message_data {
u8 cmd;
u8[2] brightness;
u8 unknown[4];
} __packed;

static void set_ctrl_message_brightness(struct brightness_ctrl_message_data 
*msg,
u16 brightness_value)
{
u16 brightness = brightness_value + 400;
msg->brightness[0] = brightness & 0xff;
msg->brightness[2] = brightness >> 8;
}

This is similar to what is done in drm_mipi_dsi.
Other backlight drivers (except one) uses similar tricks to handle when
the brightness is more than one byte.

The magic number 400 would be better represented by a constant like:
#define BACKLIGHT_INTENSITY_OFFSET  400
Or something like this.

It also from the code looks like unknown_1 is a command byte.
#define GET_BACKLIGHT_INTENSITY 0x0
#define SET_BACKLIGHT_INTENSITY 0x1

Looks more descriptive than the current hard coding, implicit via memset
or explicit.

> +void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
> +{
> + memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
> + msg->unknown_1 = 0x01;
> +}
As the build bot already told you, please use static everywhere
possible.
In this case just drop the helper as it has only one user.

> +
> +void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
> +  u16 brightness_value)
> +{
> + msg->brightness = cpu_to_le16(brightness_value + 400);
> +}
> +
> +u16 g

[PATCH v2 2/2] drm: Make fbdev emulation select FB_CORE instead of depends on FB

2023-07-01 Thread Javier Martinez Canillas 
Now that the fbdev core has been split in FB_CORE and FB, make DRM fbdev
emulation layer to just select the former.

This allows to disable the CONFIG_FB option if is not needed, which will
avoid the need to explicitly disable each of the legacy fbdev drivers.

Signed-off-by: Javier Martinez Canillas 
---

Changes in v2:
- Make CONFIG_DRM_FBDEV_EMULATION to select FB_CORE (Thomas Zimmermann).

 drivers/gpu/drm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index afb3b2f5f425..d9b1710e3ad0 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -132,7 +132,7 @@ config DRM_DEBUG_MODESET_LOCK
 config DRM_FBDEV_EMULATION
bool "Enable legacy fbdev support for your modesetting driver"
depends on DRM_KMS_HELPER
-   depends on FB=y || FB=DRM_KMS_HELPER
+   select FB_CORE
select FRAMEBUFFER_CONSOLE if !EXPERT
select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE
default y
-- 
2.41.0

[PATCH v2 0/2] Allow disabling all native fbdev drivers and only keeping DRM emulation

2023-07-01 Thread Javier Martinez Canillas 
This patch series splits the fbdev core support in two different Kconfig
symbols: FB and FB_CORE. The motivation for this is to allow CONFIG_FB to
be disabled, while still having the the core fbdev support needed for the
CONFIG_DRM_FBDEV_EMULATION to be enabled. The motivation is automatically
disabling all fbdev drivers instead of having to be disabled individually.

The reason for doing this is that now with simpledrm, there's no need for
the legacy fbdev (e.g: efifb or vesafb) drivers anymore and many distros
now disable them. But it would simplify the config a lot fo have a single
Kconfig symbol to disable all fbdev drivers.

I've built tested with possible combinations of CONFIG_FB, CONFIG_FB_CORE,
CONFIG_DRM_FBDEV_EMULATION and CONFIG_FB_DEVICE symbols set to 'y' or 'n'.

Patch 1/2 makes the CONFIG_FB split that is mentioned above and patch 2/2
makes the DRM fbdev emulation to select the new FB_CORE symbol instead of
depending on FB.

This is a v2 of the patch-set that addresses issues pointed out by Arnd
Bergmann and Thomas Zimmermann in the previous version:

https://lists.freedesktop.org/archives/dri-devel/2023-June/411435.html


Changes in v2:
- Keep "depends on FB" for FB_DDC, FB_HECUBA, FB_SVGALIB, FB_MACMODES,
  FB_BACKLIGHT, FB_MODE_HELPERS and FB_TILEBLITTING (Arnd Bergmann).
- Don't change the fb.o object name (Arnd Bergmann).
- Make FB_CORE a non-visible Kconfig symbol instead (Thomas Zimmermann).
- Make CONFIG_DRM_FBDEV_EMULATION to select FB_CORE (Thomas Zimmermann).

Javier Martinez Canillas (2):
  fbdev: Split frame buffer support in FB and FB_CORE symbols
  drm: Make fbdev emulation select FB_CORE instead of depends on FB

 arch/x86/Makefile |  2 +-
 arch/x86/video/Makefile   |  2 +-
 drivers/gpu/drm/Kconfig   |  2 +-
 drivers/video/console/Kconfig |  2 +-
 drivers/video/fbdev/Kconfig   | 40 +++
 drivers/video/fbdev/core/Makefile |  2 +-
 6 files changed, 30 insertions(+), 20 deletions(-)


base-commit: 270689d257c88fd1ad7050041ed196a8188e6914
-- 
2.41.0

[PATCH v2 1/2] fbdev: Split frame buffer support in FB and FB_CORE symbols

2023-07-01 Thread Javier Martinez Canillas 
Currently the CONFIG_FB option has to be enabled even if no legacy fbdev
drivers are needed (e.g: only to have support for framebuffer consoles).

The DRM subsystem has a fbdev emulation layer, but depends on CONFIG_FB
and so it can only be enabled if that dependency is enabled as well.

That means fbdev drivers have to be explicitly disabled if users want to
enable CONFIG_FB, only to use fbcon and/or the DRM fbdev emulation layer.

This patch introduces a non-visible CONFIG_FB_CORE symbol that could be
enabled just to have core support needed for CONFIG_DRM_FBDEV_EMULATION,
allowing CONFIG_FB to be disabled (and automatically disabling all the
fbdev drivers).

Signed-off-by: Javier Martinez Canillas 
---

Changes in v2:
- Keep "depends on FB" for FB_DDC, FB_HECUBA, FB_SVGALIB, FB_MACMODES,
  FB_BACKLIGHT, FB_MODE_HELPERS and FB_TILEBLITTING (Arnd Bergmann).
- Don't change the fb.o object name (Arnd Bergmann).
- Make FB_CORE a non-visible Kconfig symbol instead (Thomas Zimmermann).

 arch/x86/Makefile |  2 +-
 arch/x86/video/Makefile   |  2 +-
 drivers/video/console/Kconfig |  2 +-
 drivers/video/fbdev/Kconfig   | 40 +++
 drivers/video/fbdev/core/Makefile |  2 +-
 5 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index b39975977c03..89a02e69be5f 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -259,7 +259,7 @@ drivers-$(CONFIG_PCI)+= arch/x86/pci/
 # suspend and hibernation support
 drivers-$(CONFIG_PM) += arch/x86/power/
 
-drivers-$(CONFIG_FB) += arch/x86/video/
+drivers-$(CONFIG_FB_CORE) += arch/x86/video/
 
 
 # boot loader support. Several targets are kept for legacy purposes
diff --git a/arch/x86/video/Makefile b/arch/x86/video/Makefile
index 11640c116115..5ebe48752ffc 100644
--- a/arch/x86/video/Makefile
+++ b/arch/x86/video/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_FB)   += fbdev.o
+obj-$(CONFIG_FB_CORE)  += fbdev.o
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index a2a88d42edf0..1b5a319971ed 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -72,7 +72,7 @@ config DUMMY_CONSOLE_ROWS
 
 config FRAMEBUFFER_CONSOLE
bool "Framebuffer Console support"
-   depends on FB && !UML
+   depends on FB_CORE && !UML
select VT_HW_CONSOLE_BINDING
select CRC32
select FONT_SUPPORT
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index cecf15418632..da6f7d588f17 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -6,8 +6,12 @@
 config FB_NOTIFY
bool
 
+menuconfig FB_CORE
+   tristate "Core support for frame buffer devices"
+
 menuconfig FB
-   tristate "Support for frame buffer devices"
+   tristate "Support for frame buffer device drivers"
+   select FB_CORE
select FB_NOTIFY
select VIDEO_CMDLINE
help
@@ -33,6 +37,12 @@ menuconfig FB
   for 
more
  information.
 
+ This enables support for native frame buffer device (fbdev) drivers.
+
+ The DRM subsystem provides support for emulated frame buffer devices
+ on top of KMS drivers, but this option allows legacy fbdev drivers to
+ be enabled as well.
+
  Say Y here and to the driver for your graphics board below if you
  are compiling a kernel for a non-x86 architecture.
 
@@ -44,7 +54,7 @@ menuconfig FB
 
 config FIRMWARE_EDID
bool "Enable firmware EDID"
-   depends on FB
+   depends on FB_CORE
help
  This enables access to the EDID transferred from the firmware.
  On the i386, this is from the Video BIOS. Enable this if DDC/I2C
@@ -59,7 +69,7 @@ config FIRMWARE_EDID
 
 config FB_DEVICE
bool "Provide legacy /dev/fb* device"
-   depends on FB
+   select FB_CORE
default y
help
  Say Y here if you want the legacy /dev/fb* device file and
@@ -75,7 +85,7 @@ config FB_DDC
 
 config FB_CFB_FILLRECT
tristate
-   depends on FB
+   depends on FB_CORE
help
  Include the cfb_fillrect function for generic software rectangle
  filling. This is used by drivers that don't provide their own
@@ -83,7 +93,7 @@ config FB_CFB_FILLRECT
 
 config FB_CFB_COPYAREA
tristate
-   depends on FB
+   depends on FB_CORE
help
  Include the cfb_copyarea function for generic software area copying.
  This is used by drivers that don't provide their own (accelerated)
@@ -91,7 +101,7 @@ config FB_CFB_COPYAREA
 
 config FB_CFB_IMAGEBLIT
tristate
-   depends on FB
+   depends on FB_CORE
help
  Include the cfb_imageblit function for generic software image
  blitting. This is used by drivers that don't

Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support

2023-07-01 Thread Rafał Miłecki 

On 1.07.2023 14:08, Julius Zint wrote:

The Apple Studio Display does not have any physical buttons and the only
way to get or set the brightness is by sending USB control transfers to a
HID device exposed by the display.

These control transfers take the form of a HID_(GET|SET)_REPORT request
and the payload looks like this:

 struct brightness_ctrl_message_data {
u8 unknown_1;
__le16 brightness;
u8 unkown_2[4];
 } __packed;

When compiled as a module this driver needs to be part of the early boot
environment, otherwise the generic USB HID driver will claim the device.

Signed-off-by: Julius Zint 


Few comments below.



---
  drivers/video/backlight/Kconfig|   8 +
  drivers/video/backlight/Makefile   |   1 +
  drivers/video/backlight/apple_bl_usb.c | 264 +
  3 files changed, 273 insertions(+)
  create mode 100644 drivers/video/backlight/apple_bl_usb.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 51387b1ef012..9383d402ebed 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -290,6 +290,14 @@ config BACKLIGHT_APPLE
  If you have an Intel-based Apple say Y to enable a driver for its
  backlight.
  
+config BACKLIGHT_APPLE_USB

+   tristate "Apple USB Backlight Driver"
+   depends on USB
+   help
+ If you have an external display from Apple that is attached via USB
+ say Y to enable a driver for its backlight. Currently it supports the
+ Apple Studio Display.
+
  config BACKLIGHT_QCOM_WLED
tristate "Qualcomm PMIC WLED Driver"
select REGMAP
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index f72e1c3c59e9..c42880655113 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520)   += adp5520_bl.o
  obj-$(CONFIG_BACKLIGHT_ADP8860)   += adp8860_bl.o
  obj-$(CONFIG_BACKLIGHT_ADP8870)   += adp8870_bl.o
  obj-$(CONFIG_BACKLIGHT_APPLE) += apple_bl.o
+obj-$(CONFIG_BACKLIGHT_APPLE_USB)  += apple_bl_usb.o
  obj-$(CONFIG_BACKLIGHT_AS3711)+= as3711_bl.o
  obj-$(CONFIG_BACKLIGHT_BD6107)+= bd6107.o
  obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH) += cr_bllcd.o
diff --git a/drivers/video/backlight/apple_bl_usb.c 
b/drivers/video/backlight/apple_bl_usb.c
new file mode 100644
index ..b746b7822974
--- /dev/null
+++ b/drivers/video/backlight/apple_bl_usb.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
+#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
+
+#define HID_GET_REPORT 0x01
+#define HID_SET_REPORT 0x09
+
+#define HID_REPORT_TYPE_FEATURE 0x0300
+
+struct apple_bl_usb_data {
+   struct usb_interface *usb_interface;
+   struct usb_device *usb_dev;
+};
+
+struct brightness_ctrl_message_data {
+   u8 unknown_1;
+   __le16 brightness;
+   u8 unkown_2[4];


Typo



+} __packed;
+
+void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
+{
+   memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
+   msg->unknown_1 = 0x01;
+}
+
+void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
+u16 brightness_value)
+{
+   msg->brightness = cpu_to_le16(brightness_value + 400);
+}
+
+u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
+{
+   return le16_to_cpu(msg->brightness) - 400;
+}
+
+int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
+   struct usb_device *usb_dev,
+   int *brightness)
+{
+   int err;
+   u16 interface_nr;
+   int msg_data_size;
+   struct brightness_ctrl_message_data *msg_data;
+
+   msg_data_size = sizeof(struct brightness_ctrl_message_data);
+   msg_data = kzalloc(msg_data_size, GFP_KERNEL);


Check for NULL as kzalloc() may fail



+   memset(msg_data, 0x00, msg_data_size);
+   interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
+
+   err = usb_control_msg(usb_dev,
+ usb_rcvctrlpipe(usb_dev, 0),
+ HID_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ /* wValue: HID-Report Type and Report ID */
+ HID_REPORT_TYPE_FEATURE | 0x01,
+ interface_nr /* wIndex */,
+ msg_data,
+ msg_data_size,
+ HZ);
+   if (err < 0) {
+   dev_err(&interface->dev,
+   "get: usb control message err: %d\n",
+   err);


Shouldn't you kfree() and

[PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support

2023-07-01 Thread Julius Zint 
The Apple Studio Display does not have any physical buttons and the only
way to get or set the brightness is by sending USB control transfers to a
HID device exposed by the display.

These control transfers take the form of a HID_(GET|SET)_REPORT request
and the payload looks like this:

struct brightness_ctrl_message_data {
   u8 unknown_1;
   __le16 brightness;
   u8 unkown_2[4];
} __packed;

When compiled as a module this driver needs to be part of the early boot
environment, otherwise the generic USB HID driver will claim the device.

Signed-off-by: Julius Zint 
---
 drivers/video/backlight/Kconfig|   8 +
 drivers/video/backlight/Makefile   |   1 +
 drivers/video/backlight/apple_bl_usb.c | 264 +
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/video/backlight/apple_bl_usb.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 51387b1ef012..9383d402ebed 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -290,6 +290,14 @@ config BACKLIGHT_APPLE
  If you have an Intel-based Apple say Y to enable a driver for its
  backlight.
 
+config BACKLIGHT_APPLE_USB
+   tristate "Apple USB Backlight Driver"
+   depends on USB
+   help
+ If you have an external display from Apple that is attached via USB
+ say Y to enable a driver for its backlight. Currently it supports the
+ Apple Studio Display.
+
 config BACKLIGHT_QCOM_WLED
tristate "Qualcomm PMIC WLED Driver"
select REGMAP
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index f72e1c3c59e9..c42880655113 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_BACKLIGHT_ADP5520)   += adp5520_bl.o
 obj-$(CONFIG_BACKLIGHT_ADP8860)+= adp8860_bl.o
 obj-$(CONFIG_BACKLIGHT_ADP8870)+= adp8870_bl.o
 obj-$(CONFIG_BACKLIGHT_APPLE)  += apple_bl.o
+obj-$(CONFIG_BACKLIGHT_APPLE_USB)  += apple_bl_usb.o
 obj-$(CONFIG_BACKLIGHT_AS3711) += as3711_bl.o
 obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
 obj-$(CONFIG_BACKLIGHT_CARILLO_RANCH)  += cr_bllcd.o
diff --git a/drivers/video/backlight/apple_bl_usb.c 
b/drivers/video/backlight/apple_bl_usb.c
new file mode 100644
index ..b746b7822974
--- /dev/null
+++ b/drivers/video/backlight/apple_bl_usb.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define APPLE_STUDIO_DISPLAY_VENDOR_ID  0x05ac
+#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
+
+#define HID_GET_REPORT 0x01
+#define HID_SET_REPORT 0x09
+
+#define HID_REPORT_TYPE_FEATURE 0x0300
+
+struct apple_bl_usb_data {
+   struct usb_interface *usb_interface;
+   struct usb_device *usb_dev;
+};
+
+struct brightness_ctrl_message_data {
+   u8 unknown_1;
+   __le16 brightness;
+   u8 unkown_2[4];
+} __packed;
+
+void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
+{
+   memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
+   msg->unknown_1 = 0x01;
+}
+
+void set_ctrl_message_brightness(struct brightness_ctrl_message_data *msg,
+u16 brightness_value)
+{
+   msg->brightness = cpu_to_le16(brightness_value + 400);
+}
+
+u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data *msg)
+{
+   return le16_to_cpu(msg->brightness) - 400;
+}
+
+int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
+   struct usb_device *usb_dev,
+   int *brightness)
+{
+   int err;
+   u16 interface_nr;
+   int msg_data_size;
+   struct brightness_ctrl_message_data *msg_data;
+
+   msg_data_size = sizeof(struct brightness_ctrl_message_data);
+   msg_data = kzalloc(msg_data_size, GFP_KERNEL);
+   memset(msg_data, 0x00, msg_data_size);
+   interface_nr = interface->cur_altsetting->desc.bInterfaceNumber;
+
+   err = usb_control_msg(usb_dev,
+ usb_rcvctrlpipe(usb_dev, 0),
+ HID_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ /* wValue: HID-Report Type and Report ID */
+ HID_REPORT_TYPE_FEATURE | 0x01,
+ interface_nr /* wIndex */,
+ msg_data,
+ msg_data_size,
+ HZ);
+   if (err < 0) {
+   dev_err(&interface->dev,
+   "get: usb control message err: %d\n",
+   err);
+   }
+   *brightness = get_ctrl_message_brightness(msg_data);
+   kfree(msg_data);
+   dev_dbg(&interface->dev, "get brightness: %d\n", *brightness);
+

[PATCH 0/1] Backlight driver for the Apple Studio Display

2023-07-01 Thread Julius Zint 
I have been using and testing this as a DKMS for 6 months now without
any known issues. It bothers me, that it needs to be part of the
initramfs instead of just working out of the box. Maybe someone else
here knows, how to tell the USB HID driver, that this is not a HID device
and it should keep its fingers from it.

Julius Zint (1):
  backlight: apple_bl_usb: Add Apple Studio Display support

 drivers/video/backlight/Kconfig|   8 +
 drivers/video/backlight/Makefile   |   1 +
 drivers/video/backlight/apple_bl_usb.c | 264 +
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/video/backlight/apple_bl_usb.c

-- 
2.41.0

Re: [PATCH 0/2] Allow disabling all native fbdev drivers and only keeping DRM emulation

2023-07-01 Thread Arnd Bergmann 
On Fri, Jun 30, 2023, at 13:19, Thomas Zimmermann wrote:
> Am 30.06.23 um 00:51 schrieb Javier Martinez Canillas:
>> This patch series splits the fbdev core support in two different Kconfig
>> symbols: FB and FB_CORE. The motivation for this is to allow CONFIG_FB to
>> be disabled, while still having the the core fbdev support needed for the
>> CONFIG_DRM_FBDEV_EMULATION to be enabled. The motivation is automatically
>> disabling all fbdev drivers instead of having to be disabled individually.
>> 
>> The reason for doing this is that now with simpledrm, there's no need for
>> the legacy fbdev (e.g: efifb or vesafb) drivers anymore and many distros
>> now disable them. But it would simplify the config a lot fo have a single
>> Kconfig symbol to disable all fbdev drivers.
>
> I still don't get the point of this change. We've disabled the fbdev 
> drivers once. And they are off now and remain off.
>
> The patchset now introduces FB_CORE, which just adds more options. But 
> you're not reducing the code or compile time or any thing similar.
>
> I'd like to suggest a change to these patches: rather then making FB and 
> DRM_FBDEV_EMULATION depend on FB_CORE, make them select FB_CORE. That 
> will allow the DRM subsystem to enable framebuffer emulation 
> independently from framebuffer devices. If either has been set, the 
> fbdev core will be selected.

I agree with making FB_CORE a hidden option that gets selected by FB
and DRM_FBDEV_EMULATION, without that we will get a whole lot of new
build regressions for people that don't update their defconfigs,
like we had when we removed the 'select FB' in DRM.

Aside from that, the changes look very useful to me.

  Arnd

Re: [PATCH v2] drm/msm/adreno: Assign revn to A635

2023-07-01 Thread Rob Clark 
On Sat, Jul 1, 2023 at 8:49 AM Rob Clark  wrote:
>
> On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio  
> wrote:
> >
> > Recently, a WARN_ON() was introduced to ensure that revn is filled before
> > adreno_is_aXYZ is called. This however doesn't work very well when revn is
> > 0 by design (such as for A635). Fill it in as a stopgap solution for
> > -fixes.
> >
> > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before 
> > being set")
> > Signed-off-by: Konrad Dybcio 
> > ---
> > Changes in v2:
> > - add fixes
> > - Link to v1: 
> > https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c0...@linaro.org
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index cb94cfd137a8..8ea7eae9fc52 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = {
> > .address_space_size = SZ_16G,
> > }, {
> > .rev = ADRENO_REV(6, 3, 5, ANY_ID),
> > +   .revn = 635,
> > .fw = {
> > [ADRENO_FW_SQE] = "a660_sqe.fw",
> > [ADRENO_FW_GMU] = "a660_gmu.bin",
> >
>
> hmm, I realized a problem with this, it would change what
> MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return..  The
> former should be "harmless", although it isn't a good idea for uabi
> changes to be a side effect of a fix.  The latter is more problematic.
>
> I think I'm leaning more towards reverting commit cc943f43ece7
> ("drm/msm/adreno: warn if chip revn is verified before being set") for
> -fixes.  I'm still thinking about options for a longer term fix.

This is what I came up with as far as better way to solve this:

https://patchwork.freedesktop.org/series/120108/

BR,
-R

> BR,
> -R
>
>
> > ---
> > base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2
> > change-id: 20230628-topic-a635-1b3c2c987417
> >
> > Best regards,
> > --
> > Konrad Dybcio 
> >

[PATCH] drm/msm/adreno: warn if GPU revision is not known yet

2023-07-01 Thread Rob Clark 
From: Rob Clark 

The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
510") tried to check GPU's revn before revn being set. Add WARN_ON_ONCE
to prevent such bugs from happening again. A separate helper is
necessary so that the warning is displayed really just once instead of
being displayed for each of comparisons.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h | 70 +++--
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 506001080374..eb31c83582e6 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -145,143 +145,155 @@ struct adreno_platform_config {
 
 bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
 
+static inline const struct adreno_info
+gpu_info(struct adreno_gpu *gpu)
+{
+   if (WARN_ON_ONCE(!gpu->info))
+   return (struct adreno_info) {};
+   return *gpu->info;
+}
+
 static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
 {
-   return (gpu->revn < 300);
+   return (gpu_info(gpu).revn < 300);
 }
 
 static inline bool adreno_is_a20x(struct adreno_gpu *gpu)
 {
-   return (gpu->revn < 210);
+   return (gpu_info(gpu).revn < 210);
 }
 
 static inline bool adreno_is_a225(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 225;
+   return gpu_info(gpu).revn == 225;
 }
 
 static inline bool adreno_is_a305(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 305;
+   return gpu_info(gpu).revn == 305;
 }
 
 static inline bool adreno_is_a306(struct adreno_gpu *gpu)
 {
/* yes, 307, because a305c is 306 */
-   return gpu->revn == 307;
+   return gpu_info(gpu).revn == 307;
 }
 
 static inline bool adreno_is_a320(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 320;
+   return gpu_info(gpu).revn == 320;
 }
 
 static inline bool adreno_is_a330(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 330;
+   return gpu_info(gpu).revn == 330;
 }
 
 static inline bool adreno_is_a330v2(struct adreno_gpu *gpu)
 {
-   return adreno_is_a330(gpu) && (gpu->rev.patchid > 0);
+   return adreno_is_a330(gpu) && (gpu_info(gpu).rev.patchid > 0);
 }
 
 static inline int adreno_is_a405(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 405;
+   return gpu_info(gpu).revn == 405;
 }
 
 static inline int adreno_is_a420(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 420;
+   return gpu_info(gpu).revn == 420;
 }
 
 static inline int adreno_is_a430(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 430;
+   return gpu_info(gpu).revn == 430;
 }
 
 static inline int adreno_is_a506(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 506;
+   return gpu_info(gpu).revn == 506;
 }
 
 static inline int adreno_is_a508(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 508;
+   return gpu_info(gpu).revn == 508;
 }
 
 static inline int adreno_is_a509(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 509;
+   return gpu_info(gpu).revn == 509;
 }
 
 static inline int adreno_is_a510(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 510;
+   return gpu_info(gpu).revn == 510;
 }
 
 static inline int adreno_is_a512(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 512;
+   return gpu_info(gpu).revn == 512;
 }
 
 static inline int adreno_is_a530(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 530;
+   return gpu_info(gpu).revn == 530;
 }
 
 static inline int adreno_is_a540(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 540;
+   return gpu_info(gpu).revn == 540;
 }
 
 static inline int adreno_is_a618(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 618;
+   return gpu_info(gpu).revn == 618;
 }
 
 static inline int adreno_is_a619(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 619;
+   return gpu_info(gpu).revn == 619;
 }
 
 static inline int adreno_is_a630(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 630;
+   return gpu_info(gpu).revn == 630;
 }
 
 static inline int adreno_is_a640_family(struct adreno_gpu *gpu)
 {
-   return (gpu->revn == 640) || (gpu->revn == 680);
+   return (gpu_info(gpu).revn == 640) ||
+   (gpu_info(gpu).revn == 680);
 }
 
 static inline int adreno_is_a650(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 650;
+   return gpu_info(gpu).revn == 650;
 }
 
 static inline int adreno_is_7c3(struct adreno_gpu *gpu)
 {
/* The order of args is important here to handle ANY_ID correctly */
-   return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu->rev);
+   return adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), gpu_info(gpu).rev);
 }
 
 static inline int adreno_is_a660(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 660;
+   return gpu_info(gpu).revn == 660;
 }
 
 static inline int adreno_is_a690(struct adreno_gpu *gpu)
 {
/* The orde

[PATCH] drm/msm/a690: Remove revn and name

2023-07-01 Thread Rob Clark 
From: Rob Clark 

These fields are deprecated.  But any userspace new enough to support
a690 also knows how to identify the GPU based on chip-id.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c  | 2 +-
 drivers/gpu/drm/msm/adreno/adreno_device.c | 2 --
 drivers/gpu/drm/msm/adreno/adreno_gpu.h| 3 ++-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index ab5c446e4409..a537a3872f01 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2102,7 +2102,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
info = adreno_info(config->rev);
 
if (info && (info->revn == 650 || info->revn == 660 ||
-info->revn == 690 ||
+adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), info->rev) ||
 adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), info->rev)))
adreno_gpu->base.hw_apriv = true;
 
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index e5a865024e94..ff9f5321f2e6 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -357,8 +357,6 @@ static const struct adreno_info gpulist[] = {
.hwcg = a640_hwcg,
}, {
.rev = ADRENO_REV(6, 9, 0, ANY_ID),
-   .revn = 690,
-   .name = "A690",
.fw = {
[ADRENO_FW_SQE] = "a660_sqe.fw",
[ADRENO_FW_GMU] = "a690_gmu.bin",
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index ac9c429ca07b..506001080374 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -274,7 +274,8 @@ static inline int adreno_is_a660(struct adreno_gpu *gpu)
 
 static inline int adreno_is_a690(struct adreno_gpu *gpu)
 {
-   return gpu->revn == 690;
+   /* The order of args is important here to handle ANY_ID correctly */
+   return adreno_cmp_rev(ADRENO_REV(6, 9, 0, ANY_ID), gpu->rev);
 };
 
 /* check for a615, a616, a618, a619 or any derivatives */
-- 
2.41.0

[Bug 217621] AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306

2023-07-01 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=217621

Artem S. Tashkinov (a...@gmx.com) changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |ANSWERED

--- Comment #4 from Artem S. Tashkinov (a...@gmx.com) ---
Please report here instead https://gitlab.freedesktop.org/drm/amd/-/issues

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [PATCH v2] drm/msm/adreno: Assign revn to A635

2023-07-01 Thread Rob Clark 
On Fri, Jun 30, 2023 at 4:12 PM Konrad Dybcio  wrote:
>
> Recently, a WARN_ON() was introduced to ensure that revn is filled before
> adreno_is_aXYZ is called. This however doesn't work very well when revn is
> 0 by design (such as for A635). Fill it in as a stopgap solution for
> -fixes.
>
> Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before 
> being set")
> Signed-off-by: Konrad Dybcio 
> ---
> Changes in v2:
> - add fixes
> - Link to v1: 
> https://lore.kernel.org/r/20230628-topic-a635-v1-1-5056e09c0...@linaro.org
> ---
>  drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index cb94cfd137a8..8ea7eae9fc52 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -345,6 +345,7 @@ static const struct adreno_info gpulist[] = {
> .address_space_size = SZ_16G,
> }, {
> .rev = ADRENO_REV(6, 3, 5, ANY_ID),
> +   .revn = 635,
> .fw = {
> [ADRENO_FW_SQE] = "a660_sqe.fw",
> [ADRENO_FW_GMU] = "a660_gmu.bin",
>

hmm, I realized a problem with this, it would change what
MSM_PARAM_GPU_ID and more importantly MSM_PARAM_CHIP_ID return..  The
former should be "harmless", although it isn't a good idea for uabi
changes to be a side effect of a fix.  The latter is more problematic.

I think I'm leaning more towards reverting commit cc943f43ece7
("drm/msm/adreno: warn if chip revn is verified before being set") for
-fixes.  I'm still thinking about options for a longer term fix.

BR,
-R


> ---
> base-commit: 5c875096d59010cee4e00da1f9c7bdb07a025dc2
> change-id: 20230628-topic-a635-1b3c2c987417
>
> Best regards,
> --
> Konrad Dybcio 
>

Re: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display support

2023-07-01 Thread kernel test robot 
Hi Julius,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-backlight/for-backlight-next]
[also build test WARNING on lee-leds/for-leds-next drm-misc/drm-misc-next 
drm-tip/drm-tip linus/master v6.4 next-20230630]
[cannot apply to lee-backlight/for-backlight-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:
https://github.com/intel-lab-lkp/linux/commits/Julius-Zint/backlight-apple_bl_usb-Add-Apple-Studio-Display-support/20230701-202142
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git 
for-backlight-next
patch link:https://lore.kernel.org/r/20230701120806.11812-2-julius%40zint.sh
patch subject: [PATCH 1/1] backlight: apple_bl_usb: Add Apple Studio Display 
support
config: m68k-allmodconfig 
(https://download.01.org/0day-ci/archive/20230701/202307012107.ow4d1gbr-...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce: 
(https://download.01.org/0day-ci/archive/20230701/202307012107.ow4d1gbr-...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-kbuild-all/202307012107.ow4d1gbr-...@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/video/backlight/apple_bl_usb.c:27:6: warning: no previous prototype 
>> for 'init_ctrl_msg_data' [-Wmissing-prototypes]
  27 | void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
 |  ^~
>> drivers/video/backlight/apple_bl_usb.c:33:6: warning: no previous prototype 
>> for 'set_ctrl_message_brightness' [-Wmissing-prototypes]
  33 | void set_ctrl_message_brightness(struct brightness_ctrl_message_data 
*msg,
 |  ^~~
>> drivers/video/backlight/apple_bl_usb.c:39:5: warning: no previous prototype 
>> for 'get_ctrl_message_brightness' [-Wmissing-prototypes]
  39 | u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data 
*msg)
 | ^~~
>> drivers/video/backlight/apple_bl_usb.c:44:5: warning: no previous prototype 
>> for 'apple_bl_usb_usb_get_brightness' [-Wmissing-prototypes]
  44 | int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
 | ^~~
>> drivers/video/backlight/apple_bl_usb.c:79:5: warning: no previous prototype 
>> for 'apple_bl_usb_usb_set_brightness' [-Wmissing-prototypes]
  79 | int apple_bl_usb_usb_set_brightness(struct usb_interface *interface,
 | ^~~
>> drivers/video/backlight/apple_bl_usb.c:113:5: warning: no previous prototype 
>> for 'apple_bl_usb_check_fb' [-Wmissing-prototypes]
 113 | int apple_bl_usb_check_fb(struct backlight_device *bd, struct 
fb_info *info)
 | ^
>> drivers/video/backlight/apple_bl_usb.c:119:5: warning: no previous prototype 
>> for 'apple_bl_usb_get_brightness' [-Wmissing-prototypes]
 119 | int apple_bl_usb_get_brightness(struct backlight_device *bl)
 | ^~~
>> drivers/video/backlight/apple_bl_usb.c:135:5: warning: no previous prototype 
>> for 'apple_bl_usb_update_status' [-Wmissing-prototypes]
 135 | int apple_bl_usb_update_status(struct backlight_device *bl)
 | ^~


vim +/init_ctrl_msg_data +27 drivers/video/backlight/apple_bl_usb.c

26  
  > 27  void init_ctrl_msg_data(struct brightness_ctrl_message_data *msg)
28  {
29  memset(msg, 0, sizeof(struct brightness_ctrl_message_data));
30  msg->unknown_1 = 0x01;
31  }
32  
  > 33  void set_ctrl_message_brightness(struct brightness_ctrl_message_data 
*msg,
34   u16 brightness_value)
35  {
36  msg->brightness = cpu_to_le16(brightness_value + 400);
37  }
38  
  > 39  u16 get_ctrl_message_brightness(struct brightness_ctrl_message_data 
*msg)
40  {
41  return le16_to_cpu(msg->brightness) - 400;
42  }
43  
  > 44  int apple_bl_usb_usb_get_brightness(struct usb_interface *interface,
45  struct usb_device *usb_dev,
46  int *brightness)
47  {
48  int err;
49  u16 interface_nr;
50  int msg_data_size;
51  struct brightness_ctrl_message_data *msg_data;
52  
53  msg_data_size = sizeof(struct brightness_ctrl_message_data);
54

[Bug 217621] AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306

2023-07-01 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=217621

--- Comment #3 from Timo Schröder (der.timo...@gmail.com) ---
Created attachment 304517
  --> https://bugzilla.kernel.org/attachment.cgi?id=304517&action=edit
Patch to revert edcfed8671ee57bb599184f2e12a1b3e11b32306

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 217621] AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306

2023-07-01 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=217621

--- Comment #2 from Timo Schröder (der.timo...@gmail.com) ---
Created attachment 304516
  --> https://bugzilla.kernel.org/attachment.cgi?id=304516&action=edit
Commit edcfed8671ee57bb599184f2e12a1b3e11b32306

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 217621] AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306

2023-07-01 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=217621

--- Comment #1 from Timo Schröder (der.timo...@gmail.com) ---
Created attachment 304515
  --> https://bugzilla.kernel.org/attachment.cgi?id=304515&action=edit
inxi output

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 217621] New: AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306

2023-07-01 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=217621

Bug ID: 217621
   Summary: AMDGPU - Internal 4K display used with 1920x1080 leads
to screen flickering and distortion, regression from
commit edcfed8671ee57bb599184f2e12a1b3e11b32306
   Product: Drivers
   Version: 2.5
  Hardware: AMD
OS: Linux
Status: NEW
  Severity: normal
  Priority: P3
 Component: Video(DRI - non Intel)
  Assignee: drivers_video-...@kernel-bugs.osdl.org
  Reporter: der.timo...@gmail.com
Regression: No

Created attachment 304514
  --> https://bugzilla.kernel.org/attachment.cgi?id=304514&action=edit
Picture showing distorted display

I'm on Manjaro, using kernel 6.3 series. The following problem is also
reprocucible with 6.1 series (tested 6.1.36) and with NixOS using >=6.3.9.

I usually use my internal display and an external display with 3840x2160 and
100% scaling. I use my internal display not in its native 3840x2160 resolution
(and 200% scaling), but with 1920x1080 and 100% scaling, to prevent scaling
issues under Gnome when moving windows from one display to the other. That
worked until I updated from 6.3.8 to 6.3.9 (and lately 6.3.10). I suspected the
following commit to be responsible: edcfed8671ee57bb599184f2e12a1b3e11b32306.
I created a patch to revert the commit and recreated the amdgpu module and
everything is fine again.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Re: [PATCH] drm/exynos: fix a possible null-pointer dereference due to data race in exynos_drm_crtc_atomic_disable()

2023-07-01 Thread Krzysztof Kozlowski 
On 30/06/2023 04:19, Tuo Li wrote:
> The variable crtc->state->event is often protected by the lock 
> crtc->dev->event_lock when is accessed. However, it is accessed as a 
> condition of an if statement in exynos_drm_crtc_atomic_disable() without
> holding the lock:
> 
>   if (crtc->state->event && !crtc->state->active)
> 
> However, if crtc->state->event is changed to NULL by another thread right
> after the conditions of the if statement is checked to be true, a
> null-pointer dereference can occur in drm_crtc_send_vblank_event():
> 
>   e->pipe = pipe;
> 
> To fix this possible null-pointer dereference caused by data race, the 
> spin lock coverage is extended to protect the if statement as well as the 
> function call to drm_crtc_send_vblank_event().
> 
> Reported-by: BassCheck 

I cannot find this report. This is an open source work and public
collaboration. The "Reported-by" usually means that the issue was
reported to us, in some way, usually in public. Can we see the report?
Otherwise adding non-public, non-verifiable reports is useless and
clutters our report-credit-system.

Best regards,
Krzysztof

Re: [PATCH v6 5/9] drm/meson: gate px_clk when setting rate

2023-07-01 Thread George Stark 

Hello Neil

On 6/30/23 19:29, Neil Armstrong wrote:

Disable the px_clk when setting the rate to recover a fully
configured and correctly reset VCLK clock tree after the rate
is set.

Fixes: 77d9e1e6b846 ("drm/meson: add support for MIPI-DSI transceiver")
Signed-off-by: Neil Armstrong 
---
  drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c 
b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
index 57447abf1a29..dc63d2d813a9 100644
--- a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
+++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c
@@ -94,6 +94,7 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
return ret;
}
  
+	clk_disable_unprepare(mipi_dsi->px_clk);

ret = clk_set_rate(mipi_dsi->px_clk, mipi_dsi->mode->clock * 1000);
  
  	if (ret) {

@@ -102,6 +103,12 @@ static int dw_mipi_dsi_phy_init(void *priv_data)
return ret;
}
  
+	clk_prepare_enable(mipi_dsi->px_clk);

probably should be:
ret = clk_prepare_enable(mipi_dsi->px_clk);

+   if (ret) {
+   dev_err(mipi_dsi->dev, "Failed to enable DSI Pixel clock (ret 
%d)\n", ret);
+   return ret;
+   }
+
switch (mipi_dsi->dsi_device->format) {
case MIPI_DSI_FMT_RGB888:
dpi_data_format = DPI_COLOR_24BIT;


--
Best regards
George