On Mon, 30 Sep 2013 11:18:28 +0300
Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote:
> > On VLV/BYT, backlight controls a per-pipe, so when adjusting the
> > backlight we need to pass the correct info. So make the externally
> > visible backlight functions take a connector argument, which can be used
> > internally to figure out the pipe backlight to adjust.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 8 +++++
> > drivers/gpu/drm/i915/intel_dp.c | 5 ++-
> > drivers/gpu/drm/i915/intel_drv.h | 8 +++--
> > drivers/gpu/drm/i915/intel_lvds.c | 9 +++--
> > drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++-
> > drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++--------------
> > 6 files changed, 88 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9a7136c..d6a1868 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > }
> >
> > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> > +{
> > + struct intel_encoder *encoder = connector->encoder;
> > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> > +
> > + return crtc->pipe;
>
> What if crtc == NULL?
>
> I guess this shouldn't happen on the backlight enable/disable call paths
> through encoder callbacks, but what about backlight sysfs and opregion?
Yeah, it's definitely worth checking on a generic function like this.
I suppose we should return a negative value in that case and make sure
the callers check for it.
> > + /*
> > + * Could match the OpRegion connector here instead, but we'd
> > + * also need to verify the connector could handle a backlight
> > + * call.
> > + */
>
> Connector callbacks for backlight is the future?
Yeah we definitely need to deal with this case better somehow...
>
> > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> > + if (encoder->crtc == crtc) {
> > + found = true;
> > + break;
> > + }
> > +
> > + if (!found)
> > + return ASLC_BACKLIGHT_FAILED;
> > +
> > + list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > + if (connector->encoder == encoder)
> > + intel_connector = to_intel_connector(connector);
> > +
> > + if (!found)
> > + return ASLC_BACKLIGHT_FAILED;
>
> ITYM if (intel_connector == NULL).
Oops yeah, cult & paste fail.
> It aches a little to realize you pick a fixed pipe here, go through all
> the trouble of finding the corresponding connector, only to have
> intel_panel_set_backlight() dig out the pipe from the connector again...
>
> An alternative (not necessarily better, just different) approach might
> be changing backlight on all enabled pipes on requests coming from
> opregion or sysfs (and maybe disabling backlight on disabled
> pipes). This would let the backlight be adjusted via sysfs/opregion also
> on pipe B, which doesn't happen with the proposed patch.
Yeah that might be better; I don't know how real hw will be wired up,
but on the prototype board I have that would work fine (and should
continue to work until someone builds a device with two panels and want
independent backlight control).
> > u32 max;
> >
> > - max = i915_read_blc_pwm_ctl(dev);
> > + max = i915_read_blc_pwm_ctl(dev, pipe);
>
> Unrelated to this patch:
>
> This reminds me again we should probably just pick an arbitrary
> backlight range we expose to the userspace, and scale right before we
> write the value to the duty cycle. There's a (somewhat theoretical)
> chance the two backlight PWMs happen to have different max values, and
> the max we expose shouldn't change depending on the pipe. Also we can't
> support changing the PWM frequency if we expose that as the backlight
> device max value.
Yeah that's a good point. Yet another thing to clean up in the
backlight code.
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
