lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] drm/dsi-mipi: Fix byte order of DCS set/get brightness
Hi Richard,
On Fri, Jan 13, 2023 at 04:24:23PM -0500, Richard Acayan wrote:
> On Fri, Jan 13, 2023 at 05:34:55PM +0100, Sam Ravnborg wrote:
> > Hi Richard/Daniel.
> Not sure if you saw the original commit that I linked.
>
> This patch was written in 2017 for the Pixel 3 and Pixel 3 XL
> smartphones. I didn't contact the original author before sending this,
> since the kernel that this patch originated from is EOL.
>
> This greeting is still fine because it would be great if the original
> author took responsibility for getting this patch applied.
>
> >
> > On Thu, Jan 12, 2023 at 11:18:48PM -0500, Richard Acayan wrote:
> >> From: Daniel Mentz <danielmentz@google.com>
> >>
> >> The MIPI DCS specification demands that brightness values are sent in
> >> big endian byte order. It also states that one parameter (i.e. one byte)
> >> shall be sent/received for 8 bit wide values, and two parameters shall
> >> be used for values that are between 9 and 16 bits wide.
> > It is only a few week ago someone write they needed a byteswap to use
> > the kernel provided mipi_dsi_dcs_set_display_brightness(). I did not
> > realize then that this is because it is buggy.
> I didn't even consider sending this patch when I initially saw it. I
> would have performed the byte swap in a new S6E3FA7 panel driver just
> like in SOFEF00.
>
> >
> > Some comments on the patch:
> > - It would be nice to split it up so first patch fixes that the
> > parameters are in big-endian. This would go for both set and get.
> The brightness is only big endian if it's 16-bit.
>
> Most MIPI panel drivers in the kernel tree only send an 8-bit
> brightness, or none at all. For these panels, the brightness can be sent
> as a 16-bit integer in little endian, as it has the same effect as an
> 8-bit integer, with a zero byte appended. The original MIPI brightness
> functions work properly with no changes for 8-bit brightness panels
> only.
>
> If we want separate 8-bit (original) and 16-bit functions, the 8-bit
> functions shouldn't pass the brightness in big endian. That doesn't mean
> we shouldn't change them at all.
Agree, the brightness is passed exactly as returned by
backlight_get_brightness(), and the helper take care of the correct byte
ordering.

>
> >
> > - Second patch could then introduce the possibility to a driver to
> > use a variant that matches the display.
> > I would suggest to go for two functions:
> > mipi_dsi_dcs_set_display_brightness()
> > mipi_dsi_dcs_set_display_brightness_two()
> I'm thinking of a "_large" or "_wide" suffix instead. This would make a
> 32-bit suffix tricky though, if it is ever needed.
_large is way better than _two, and would be my choice.

>
> > So it is obvious when you go for the two byte version without adding a
> > bool parameter.
> > If the implementation is done using a helper or two independent
> > functions are up to you.
> These get_display_brightness and set_display_brightness functions are
> already helpers. I'll have them duplicated.
>
> >
> > The patch split would be nice, but at least we should have two functions
> > and not a bool parameter.
> Yes, that was a bad idea on my part. With separate functions for 16-bit
> brightness, there is no API change, less panel driver touching, and
> panel driver changes can happen in separate patches. Thank you for the
> suggestion.
Great, looks forward for the patches.

Sam

\
 
 \ /
  Last update: 2023-03-26 23:41    [W:0.227 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site