Re: [PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
On Tue, 21 May 2024, Ville Syrjälä wrote: > On Tue, May 21, 2024 at 12:51:03PM +0300, Jani Nikula wrote: >> On Mon, 20 May 2024, Ville Syrjälä wrote: >> > On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: >> >> On Fri, 17 May 2024, Ville Syrjala wrote: >> >> > From: Ville Syrjälä >> >> > >> >> > Extract a helper to check whether the source+sink combo >> >> > supports DSC. That basic check is needed both during mode >> >> > validation and compute config. We'll also need to add extra >> >> > checks to both places, so having a single place for it is nicer. >> >> > >> >> > Signed-off-by: Ville Syrjälä >> >> > --- >> >> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++-- >> >> > 1 file changed, 14 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >> >> > b/drivers/gpu/drm/i915/display/intel_dp.c >> >> > index 1e88449fe5f2..7bf283b4df7f 100644 >> >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> >> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp >> >> > *intel_dp, >> >> >connector->force_bigjoiner_enable; >> >> > } >> >> > >> >> > +static bool intel_dp_has_dsc(struct intel_connector *connector) >> >> >> >> Why not const? >> > >> > We've generally not consted these things. And then whenver add >> > one const somewhere it usually ends up getting in the way later, >> > not because we need mutability but simply because we want to >> > call something that doesn't have the const. >> > >> > I suppose if we do want to start consting things more we should >> > just do some kind of bigger pass over the whole codebase so that >> > that there's less chance of pain later. >> > >> > We're also not using container_of_const() for these right now, >> > so the const can vanish semi-accidentally when casting things. >> > >> > I suppose this thing might be low level enough that the const >> > could be kept. I'll have another think about it. >> >> It's just that this series drops a bunch of const because of this, which >> feels like the opposite of what you usually do. :) > > I suppose. > > My current rule of thumb is: > - atomic object states and fbs should be const if possible > - everything else is not > > I wouldn't mind making more things const, but I suspect > there are several sizeable rabbit holes that need to be > dug out beforehand. Fair enough. Like I said, the series is R-b. J. -- Jani Nikula, Intel
Re: [PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
On Tue, May 21, 2024 at 12:51:03PM +0300, Jani Nikula wrote: > On Mon, 20 May 2024, Ville Syrjälä wrote: > > On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote: > >> On Fri, 17 May 2024, Ville Syrjala wrote: > >> > From: Ville Syrjälä > >> > > >> > Extract a helper to check whether the source+sink combo > >> > supports DSC. That basic check is needed both during mode > >> > validation and compute config. We'll also need to add extra > >> > checks to both places, so having a single place for it is nicer. > >> > > >> > Signed-off-by: Ville Syrjälä > >> > --- > >> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++-- > >> > 1 file changed, 14 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> > b/drivers/gpu/drm/i915/display/intel_dp.c > >> > index 1e88449fe5f2..7bf283b4df7f 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp > >> > *intel_dp, > >> > connector->force_bigjoiner_enable; > >> > } > >> > > >> > +static bool intel_dp_has_dsc(struct intel_connector *connector) > >> > >> Why not const? > > > > We've generally not consted these things. And then whenver add > > one const somewhere it usually ends up getting in the way later, > > not because we need mutability but simply because we want to > > call something that doesn't have the const. > > > > I suppose if we do want to start consting things more we should > > just do some kind of bigger pass over the whole codebase so that > > that there's less chance of pain later. > > > > We're also not using container_of_const() for these right now, > > so the const can vanish semi-accidentally when casting things. > > > > I suppose this thing might be low level enough that the const > > could be kept. I'll have another think about it. > > It's just that this series drops a bunch of const because of this, which > feels like the opposite of what you usually do. :) I suppose. My current rule of thumb is: - atomic object states and fbs should be const if possible - everything else is not I wouldn't mind making more things const, but I suspect there are several sizeable rabbit holes that need to be dug out beforehand. -- Ville Syrjälä Intel
Re: [PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
On Mon, 20 May 2024, Ville Syrjälä wrote:
> On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote:
>> On Fri, 17 May 2024, Ville Syrjala wrote:
>> > From: Ville Syrjälä
>> >
>> > Extract a helper to check whether the source+sink combo
>> > supports DSC. That basic check is needed both during mode
>> > validation and compute config. We'll also need to add extra
>> > checks to both places, so having a single place for it is nicer.
>> >
>> > Signed-off-by: Ville Syrjälä
>> > ---
>> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++--
>> > 1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 1e88449fe5f2..7bf283b4df7f 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp
>> > *intel_dp,
>> > connector->force_bigjoiner_enable;
>> > }
>> >
>> > +static bool intel_dp_has_dsc(struct intel_connector *connector)
>>
>> Why not const?
>
> We've generally not consted these things. And then whenver add
> one const somewhere it usually ends up getting in the way later,
> not because we need mutability but simply because we want to
> call something that doesn't have the const.
>
> I suppose if we do want to start consting things more we should
> just do some kind of bigger pass over the whole codebase so that
> that there's less chance of pain later.
>
> We're also not using container_of_const() for these right now,
> so the const can vanish semi-accidentally when casting things.
>
> I suppose this thing might be low level enough that the const
> could be kept. I'll have another think about it.
It's just that this series drops a bunch of const because of this, which
feels like the opposite of what you usually do. :)
BR,
Jani.
>
>>
>> > +{
>> > + struct drm_i915_private *i915 = to_i915(connector->base.dev);
>> > +
>> > + if (!HAS_DSC(i915))
>> > + return false;
>> > +
>> > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd))
>> > + return false;
>> > +
>> > + return true;
>> > +}
>> > +
>> > static enum drm_mode_status
>> > intel_dp_mode_valid(struct drm_connector *_connector,
>> >struct drm_display_mode *mode)
>> > @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>> >mode_rate = intel_dp_link_required(target_clock,
>> >
>> > intel_dp_mode_min_output_bpp(connector, mode));
>> >
>> > - if (HAS_DSC(dev_priv) &&
>> > - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) {
>> > + if (intel_dp_has_dsc(connector)) {
>> >enum intel_output_format sink_format, output_format;
>> >int pipe_bpp;
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
Re: [PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
On Mon, May 20, 2024 at 01:47:34PM +0300, Jani Nikula wrote:
> On Fri, 17 May 2024, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > Extract a helper to check whether the source+sink combo
> > supports DSC. That basic check is needed both during mode
> > validation and compute config. We'll also need to add extra
> > checks to both places, so having a single place for it is nicer.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 16 ++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 1e88449fe5f2..7bf283b4df7f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp
> > *intel_dp,
> >connector->force_bigjoiner_enable;
> > }
> >
> > +static bool intel_dp_has_dsc(struct intel_connector *connector)
>
> Why not const?
We've generally not consted these things. And then whenver add
one const somewhere it usually ends up getting in the way later,
not because we need mutability but simply because we want to
call something that doesn't have the const.
I suppose if we do want to start consting things more we should
just do some kind of bigger pass over the whole codebase so that
that there's less chance of pain later.
We're also not using container_of_const() for these right now,
so the const can vanish semi-accidentally when casting things.
I suppose this thing might be low level enough that the const
could be kept. I'll have another think about it.
>
> > +{
> > + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > +
> > + if (!HAS_DSC(i915))
> > + return false;
> > +
> > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > static enum drm_mode_status
> > intel_dp_mode_valid(struct drm_connector *_connector,
> > struct drm_display_mode *mode)
> > @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> > mode_rate = intel_dp_link_required(target_clock,
> >
> > intel_dp_mode_min_output_bpp(connector, mode));
> >
> > - if (HAS_DSC(dev_priv) &&
> > - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) {
> > + if (intel_dp_has_dsc(connector)) {
> > enum intel_output_format sink_format, output_format;
> > int pipe_bpp;
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
Re: [PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
On Fri, 17 May 2024, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Extract a helper to check whether the source+sink combo
> supports DSC. That basic check is needed both during mode
> validation and compute config. We'll also need to add extra
> checks to both places, so having a single place for it is nicer.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 1e88449fe5f2..7bf283b4df7f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp,
> connector->force_bigjoiner_enable;
> }
>
> +static bool intel_dp_has_dsc(struct intel_connector *connector)
Why not const?
> +{
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> +
> + if (!HAS_DSC(i915))
> + return false;
> +
> + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd))
> + return false;
> +
> + return true;
> +}
> +
> static enum drm_mode_status
> intel_dp_mode_valid(struct drm_connector *_connector,
> struct drm_display_mode *mode)
> @@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> mode_rate = intel_dp_link_required(target_clock,
>
> intel_dp_mode_min_output_bpp(connector, mode));
>
> - if (HAS_DSC(dev_priv) &&
> - drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) {
> + if (intel_dp_has_dsc(connector)) {
> enum intel_output_format sink_format, output_format;
> int pipe_bpp;
--
Jani Nikula, Intel
[PATCH 2/7] drm/i915: Extract intel_dp_has_dsc()
From: Ville Syrjälä
Extract a helper to check whether the source+sink combo
supports DSC. That basic check is needed both during mode
validation and compute config. We'll also need to add extra
checks to both places, so having a single place for it is nicer.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_dp.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 1e88449fe5f2..7bf283b4df7f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1220,6 +1220,19 @@ bool intel_dp_need_bigjoiner(struct intel_dp *intel_dp,
connector->force_bigjoiner_enable;
}
+static bool intel_dp_has_dsc(struct intel_connector *connector)
+{
+ struct drm_i915_private *i915 = to_i915(connector->base.dev);
+
+ if (!HAS_DSC(i915))
+ return false;
+
+ if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd))
+ return false;
+
+ return true;
+}
+
static enum drm_mode_status
intel_dp_mode_valid(struct drm_connector *_connector,
struct drm_display_mode *mode)
@@ -1274,8 +1287,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
mode_rate = intel_dp_link_required(target_clock,
intel_dp_mode_min_output_bpp(connector, mode));
- if (HAS_DSC(dev_priv) &&
- drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) {
+ if (intel_dp_has_dsc(connector)) {
enum intel_output_format sink_format, output_format;
int pipe_bpp;
--
2.44.1
