RE: [PATCH] drm/nouveau/kms/nv50-: fix build failure with CONFIG_BACKLIGHT=n
[AMD Official Use Only]
+Harry
-Original Message-
From: Daniel Vetter
Sent: Friday, July 23, 2021 11:23 AM
To: Arnd Bergmann
Cc: Daniel Vetter ; Ben Skeggs ; David
Airlie ; Lyude Paul ; Arnd Bergmann
; Ville Syrjälä ; Cornij, Nikola
; dri-devel ; Nouveau
Dev ; Linux Kernel Mailing List
Subject: Re: [PATCH] drm/nouveau/kms/nv50-: fix build failure with
CONFIG_BACKLIGHT=n
On Fri, Jul 23, 2021 at 12:16:31PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 23, 2021 at 11:25 AM Daniel Vetter wrote:
> >
> > On Fri, Jul 23, 2021 at 11:15 AM Arnd Bergmann wrote:
> > >
> > > From: Arnd Bergmann
> > >
> > > When the backlight support is disabled, the driver fails to build:
> > >
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c: In function
> > > 'nv50_sor_atomic_disable':
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c:1665:59: error: 'struct
> > > nouveau_connector' has no member named 'backlight'
> > > 1665 | struct nouveau_backlight *backlight =
> > > nv_connector->backlight;
> > > | ^~
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c:1670:35: error: invalid use of
> > > undefined type 'struct nouveau_backlight'
> > > 1670 | if (backlight && backlight->uses_dpcd) {
> > > | ^~
> > > drivers/gpu/drm/nouveau/dispnv50/disp.c:1671:64: error: invalid use of
> > > undefined type 'struct nouveau_backlight'
> > > 1671 | ret = drm_edp_backlight_disable(aux,
> > > >edp_info);
> > > |^~
> > >
> > > The patch that introduced the problem already contains some #ifdef
> > > checks, so just add another one that makes it build again.
> > >
> > > Fixes: 6eca310e8924 ("drm/nouveau/kms/nv50-: Add basic DPCD
> > > backlight support for nouveau")
> > > Signed-off-by: Arnd Bergmann
> >
> > Can we just toss the idea that BACKTLIGHT=n is a reasonable config
> > for drm drivers using backlights, and add depends BACKLIGHT to all
> > of them?
> >
> > I mean this is a perfect source of continued patch streams to keep
> > us all busy, but beyond that I really don't see the point ... I
> > frankly have better things to do, and especially with the big
> > drivers we have making backlight optional saves comparitively nothing.
> > -Daniel
>
> Yes! I'd definitely be in favor of that, I've wasted way too much time
> trying to sort through dependency loops and other problems with backlight
> support.
>
> Maybe we should leave the drivers/video/fbdev/ drivers untouched in
> this regard, at least for the moment, but for the drivers/gpu/drm
> users of backlight that would be a nice simplification, and even the
> smallest ones are unlikely to be used on systems that are too memory
> constrained to deal with 4KB extra .text.
Yeah I think we can do this entirely ad-hoc, i.e. any time the backlight wheel
wobbles off again we nail it down for good for that driver with a depends on
BACKGLIGHT and remove any lingering #ifdef all over.
If you want maybe start out with the biggest drm drivers in a series, I think
if nouveau/amdgpu/i915 folks ack this you're good to go to just convert as
things get in the way.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2Fdata=04%7C01%7Cnikola.cornij%40amd.com%7C48104ef3c5724531e8f708d94dedcf83%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637626506075627495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=KWVzpQyZsBh4jo522b36pru1zPBxxW2wvXUAjJ6u3G8%3Dreserved=0
RE: [PATCH v2 1/1] drm/i915: Use the correct max source link rate for MST
[AMD Official Use Only - Internal Distribution Only]
I'll fix the dpcd part to use kHz on Monday
My apologies as well, not only for coming up with the wrong patch in first
place, but also for missing to CC all the maintainers.
-Original Message-
From: Lyude Paul
Sent: Friday, April 30, 2021 6:41 PM
To: Cornij, Nikola ; amd-...@lists.freedesktop.org
Cc: Pillai, Aurabindo ; Lipski, Mikita
; ville.syrj...@linux.intel.com; koba...@canonical.com;
intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
jani.nik...@linux.intel.com; bske...@redhat.com
Subject: Re: [PATCH v2 1/1] drm/i915: Use the correct max source link rate for
MST
Alright - I had Ville double check this and give their A-B on IRC (I just need
to fix the open coded link_rate_to_bw() here). Since this got broken on drm-
misc-next I'm going to go ahead and push the fix there, since I'm not going to
be around next Monday or Tuesday and I don't want to leave i915 broken in the
interim. Sorry about the noise with this Jani!
As well - I'll get to fixing the dpcd unit usage once I get back on Wednesday
(unless Nikola's already gotten to it by then) so we use KHz instead. Although
as ville has pointed out, I think we should teach the topology manager to allow
passing for the current link rate/lane count during state computation in the
long term.
On Fri, 2021-04-30 at 17:45 -0400, Nikola Cornij wrote:
> [why]
> Previously used value was not safe to provide the correct value, i.e.
> it could be 0 if not not configured, leading to no MST on this platform.
>
> [how]
> Do not use the value from BIOS, but from the structure populated at
> encoder initialization time.
>
> Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver
> Capability DPCD
> space")
> Signed-off-by: Nikola Cornij
> Reviewed-by: Lyude Paul
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bf7f8487945c..3642d7578658 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -942,7 +942,7 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
> struct intel_dp *intel_dp = _port->dp;
> enum port port = dig_port->base.port;
> int ret;
> - int bios_max_link_rate;
> + int max_source_rate = intel_dp->source_rates[intel_dp-
> >num_source_rates - 1];
>
> if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
> return 0;
> @@ -957,11 +957,11 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
>
> /* create encoders */
> intel_dp_create_fake_mst_encoders(dig_port);
> - bios_max_link_rate =
> intel_bios_dp_max_link_rate(_port->base);
> ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr,
> >drm,
>_dp->aux, 16, 3,
>(u8)dig_port->max_lanes,
> - (u8)(bios_max_link_rate /
> 27000), conn_base_id);
> + (u8)(max_source_rate /
> +27000),
> + conn_base_id);
> if (ret)
> return ret;
>
--
Sincerely,
Lyude Paul (she/her)
Software Engineer at Red Hat
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to
check on my status. I don't bite!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH] drm/dsc: Return unsigned long on compute offset
If you're going to make all of them the same, then u64, please.
This is because I'm not sure if calculations require 64-bit at some stage.
-Original Message-
From: Lipski, Mikita
Sent: November 19, 2019 10:08 AM
To: Ville Syrjälä ; Lipski, Mikita
Cc: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Cornij,
Nikola
Subject: Re: [PATCH] drm/dsc: Return unsigned long on compute offset
On 19/11/2019 09:56, Ville Syrjälä wrote:
> On Tue, Nov 19, 2019 at 09:45:26AM -0500, mikita.lip...@amd.com wrote:
>> From: Mikita Lipski
>>
>> We shouldn't compare int with unsigned long to find the max value and
>> since we are not expecting negative value returned from
>> compute_offset we should make this function return unsigned long so
>> we can compare the values when computing rc parameters.
>
> Why are there other unsigned longs in dsc parameter computation in the
> first place?
I believe it was initially set to be unsigned long for variable consistency,
when we ported intel_compute_rc_parameters into drm_dsc_compute_rc_parameters.
But now that I look at it, we can actually just set them to u32 or u64, as
nothing should exceed that.
>
>>
>> Cc: Nikola Cornij
>> Cc: Harry Wentland
>> Signed-off-by: Mikita Lipski
>> ---
>> drivers/gpu/drm/drm_dsc.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
>> index 74f3527f567d..ec40604ab6a2 100644
>> --- a/drivers/gpu/drm/drm_dsc.c
>> +++ b/drivers/gpu/drm/drm_dsc.c
>> @@ -245,11 +245,11 @@ void drm_dsc_pps_payload_pack(struct
>> drm_dsc_picture_parameter_set *pps_payload,
>> }
>> EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>>
>> -static int compute_offset(struct drm_dsc_config *vdsc_cfg, int
>> pixels_per_group,
>> +static unsigned long compute_offset(struct drm_dsc_config *vdsc_cfg, int
>> pixels_per_group,
>> int groups_per_line, int grpcnt)
>> {
>> -int offset = 0;
>> -int grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
>> pixels_per_group);
>> +unsigned long offset = 0;
>> +unsigned long grpcnt_id = DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay,
>> pixels_per_group);
>>
>> if (grpcnt <= grpcnt_id)
>> offset = DIV_ROUND_UP(grpcnt * pixels_per_group *
>> vdsc_cfg->bits_per_pixel, 16);
>> --
>> 2.17.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lip...@amd.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
