[Intel-gfx] [PATCH 1/2] drm/i915: make backlight functions take a connector

[Jesse Barnes]

On Mon, 30 Sep 2013 11:18:28 +0300
Jani Nikula <jani.nikula at linux.intel.com> wrote:

> On Sat, 28 Sep 2013, Jesse Barnes <jbarnes at virtuousgeek.org> 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 at virtuousgeek.org>
> > ---
> >  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