Re: [PATCH v2 1/2] drm/nouveau/disp: add backlight for ada lovelace

[Angelo Dureghello <angelo@xxxxxxxxxxxxxxxx>]

Hi Lyude,

thanks a lot for your reply,

On 15/04/24 6:54 PM, Lyude Paul wrote:
> Hm. Could you share some logs with drm.debug=0x116? I'm a bit confused
> because I would have thought that we were already probing for eDP
> backlights seeing as I added support for them at one point?
>
> (I hope this isn't the point I learn I actually forgot to add support
> for them :P)
i am attaching 2 dmesg logs with drm.debug=0x116, with these 2 patches
applied to mainstream, and without them.

https://pastecode.io/s/fyjkky6y
https://pastecode.io/s/gdf8v3e3


Note, this laptop (Lenovo Legion 5 Pro 16ARX8) has 2 video cards,
amd (cpu built-in), and additional nvidia, it can work in "switchable"
mode, where amndgpu backlight works, and in discrete mode (as now),
using only nvidia gpu.

Main issue is that in nouveau_backlight.c
nouveau_backlight_init() is actually not considering ADA LOVELACE in
the main switch/case. So execution falls back to acpi backlight,
that for this laptop does not work at all (tried vendor, native,
video). Tested also nvidia-wmi-ec-backlight, the module does not
even get loaded. Only solution i found is this patchset.


> On Fri, 2024-04-12 at 21:49 +0200, Angelo Dureghello wrote:
> > Add working backlight for "ada lovelace" missing case.
> >
> > The nvif method is actually not working for this chipset so
> > used the drm apis. Also, by dpcd, drm layer is calculating a
> > max brightness of 255, but to get a real correct max brightnes
> > the maximum must be multiplied by a factor of 16.
> >
> > Tested to work properly in Legion Lenovo Pro 5
> >
> > Lenovo Legion 5 Pro 16ARX8
> > Bios ver LPCN49WW
> > 	 LPEC49WW
> > SN PF4T63AZ
> > Nvidia RTX4060 MaxQ/Mobile rev a1 (ADA LOVELACE AD107M)
> > AMD Ryzen 9 7945HX + Radeon
> >
> > and wayland.
> >
> > ---
> > Changes for v2:
> > - add some comments
> > - remove x16 multiplication (hack)
> > - remove forgot debug printk
> >
> > Signed-off-by: Angelo Dureghello <angelo@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/nouveau/nouveau_backlight.c | 54
> > +++++++++++++++++++++
> >   1 file changed, 54 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > index d47442125fa1..7b7306d18ad7 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > @@ -286,6 +286,56 @@ nv50_backlight_init(struct nouveau_backlight
> > *bl,
> >   	return 0;
> >   }
> >   
> > +static int
> > +nv19x_backlight_init(struct nouveau_backlight *bl,
> > +		     struct nouveau_connector *nv_conn,
> > +		     struct nouveau_encoder *nv_encoder,
> > +		     struct backlight_properties *props,
> > +		     const struct backlight_ops **ops)
> > +{
> > +	int ret;
> > +	u16 current_level;
> > +	u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
> > +	u8 current_mode;
> > +	struct nouveau_drm *drm = nouveau_drm(nv_encoder-
> > > base.base.dev);
> > +
> > +	/*
> > +	 * nvif functions, so also nvif_outp_bl_get, are not working
> > with this
> > +	 * connector (return -22), using only drm layer.
> > +	 */
> > +	if (nv_conn->type == DCB_CONNECTOR_eDP) {
> > +
> > +		ret = drm_dp_dpcd_read(&nv_conn->aux,
> > DP_EDP_DPCD_REV, edp_dpcd,
> > +				       EDP_DISPLAY_CTL_CAP_SIZE);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (!drm_edp_backlight_supported(edp_dpcd))
> > +			return -ENODEV;
> > +
> > +		ret = drm_edp_backlight_init(&nv_conn->aux, &bl-
> > > edp_info, 0, edp_dpcd,
> > +					 &current_level,
> > &current_mode);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = drm_edp_backlight_enable(&nv_conn->aux, &bl-
> > > edp_info, current_level);
> > +		if (ret < 0) {
> > +			NV_ERROR(drm, "Failed to enable backlight on
> > %s: %d\n",
> > +				 nv_conn->base.name, ret);
> > +			return ret;
> > +		}
> > +
> > +		*ops = &nv50_edp_bl_ops;
> > +
> > +		props->max_brightness = bl->edp_info.max;
> > +		props->brightness = current_level;
> > +		bl->uses_dpcd = true;
> > +
> > +		return 0;
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> >   int
> >   nouveau_backlight_init(struct drm_connector *connector)
> >   {
> > @@ -332,6 +382,10 @@ nouveau_backlight_init(struct drm_connector
> > *connector)
> >   		ret = nv50_backlight_init(bl,
> > nouveau_connector(connector),
> >   					  nv_encoder, &props, &ops);
> >   		break;
> > +	case NV_DEVICE_INFO_V0_ADA:
> > +		ret = nv19x_backlight_init(bl,
> > nouveau_connector(connector),
> > +					   nv_encoder, &props,
> > &ops);
> > +		break;
> >   	default:
> >   		ret = 0;
> >   		goto fail_alloc;
> Regards,
> angelo