Re: [PATCH 2/2] drm: xlnx: consolidate the functions which programming AUDIO_VIDEO_SELECT register

["quanyang.wang" <quanyang.wang@windriver.com>]

Hi Paul,
Thank you for your review.
I will update all of this in V2.
Thanks,
Quanyang

On 5/18/21 1:09 AM, Paul Cercueil wrote:
> Hi,
> 
> Le jeu., mai 13 2021 at 19:45:40 +0800, quanyang.wang@windriver.com a 
> écrit :
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> For now, the functions zynqmp_disp_avbuf_enable/disable_audio and
>> zynqmp_disp_avbuf_enable/disable_video are all programming the register
>> AV_BUF_OUTPUT_AUDIO_VIDEO_SELECT to select the output for audio or video.
>> And in the future, many drm properties (like video_tpg, audio_tpg,
>> audio_pl, etc) also need to access it. So let's introduce some variables
>> of enum type and consolidate the code to unify handling this.
>>
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>> ---
>>  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 166 ++++++++++++++----------
>>  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h |  15 +--
>>  2 files changed, 101 insertions(+), 80 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c 
>> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index c55e24412f8c..a82bc88a98aa 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -102,12 +102,39 @@ enum zynqmp_disp_layer_id {
>>
>>  /**
>>   * enum zynqmp_disp_layer_mode - Layer mode
>> - * @ZYNQMP_DISP_LAYER_NONLIVE: non-live (memory) mode
>> + * @ZYNQMP_DISP_LAYER_MEM: memory mode
>>   * @ZYNQMP_DISP_LAYER_LIVE: live (stream) mode
>> + * @ZYNQMP_DISP_LAYER_TPG: tpg mode (only for video layer)
>> + * @ZYNQMP_DISP_LAYER_DISABLE: disable mode
>>   */
>>  enum zynqmp_disp_layer_mode {
>> -    ZYNQMP_DISP_LAYER_NONLIVE,
>> -    ZYNQMP_DISP_LAYER_LIVE
>> +    ZYNQMP_DISP_LAYER_MEM,
>> +    ZYNQMP_DISP_LAYER_LIVE,
>> +    ZYNQMP_DISP_LAYER_TPG,
>> +    ZYNQMP_DISP_LAYER_DISABLE
>> +};
>> +
>> +enum avbuf_vid_mode {
>> +    VID_MODE_LIVE,
>> +    VID_MODE_MEM,
>> +    VID_MODE_TPG,
>> +    VID_MODE_NONE
>> +};
>> +
>> +enum avbuf_gfx_mode {
>> +    GFX_MODE_DISABLE,
>> +    GFX_MODE_MEM,
>> +    GFX_MODE_LIVE,
>> +    GFX_MODE_NONE
>> +};
>> +
>> +enum avbuf_aud_mode {
>> +    AUD1_MODE_LIVE,
>> +    AUD1_MODE_MEM,
>> +    AUD1_MODE_TPG,
>> +    AUD1_MODE_DISABLE,
>> +    AUD2_MODE_DISABLE,
>> +    AUD2_MODE_ENABLE
>>  };
>>
>>  /**
>> @@ -542,92 +569,98 @@ static void 
>> zynqmp_disp_avbuf_disable_channels(struct zynqmp_disp_avbuf *avbuf)
>>  }
>>
>>  /**
>> - * zynqmp_disp_avbuf_enable_audio - Enable audio
>> + * zynqmp_disp_avbuf_output_select - Select the buffer manager outputs
>>   * @avbuf: Audio/video buffer manager
>> + * @layer: The layer
>> + * @mode: The mode for this layer
>>   *
>> - * Enable all audio buffers with a non-live (memory) source.
>> + * Select the buffer manager outputs for @layer.
>>   */
>> -static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf 
>> *avbuf)
>> +static void zynqmp_disp_avbuf_output_select(struct zynqmp_disp_avbuf 
>> *avbuf,
>> +                       struct zynqmp_disp_layer *layer, u32 mode)
> 
> You can put 'mode' on a new line to avoid getting over 80 characters.
> 
>>  {
>> -    u32 val;
>> -
>> -    val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -    val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> -    val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM;
>> -    val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
>> -    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +    u32 reg;
>> +
>> +    reg = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
> 
> Empty line here (spacing before comment)
> 
>> +    /* Select audio mode when the layer is NULL */
>> +    if (layer == NULL) {
>> +        if (mode >= AUD2_MODE_DISABLE) {
>> +            reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK;
>> +            reg |= (mode - AUD2_MODE_DISABLE)
>> +                << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT;
> 
> Please consider using the FIELD_PREP() macro from <linux/bitfield.h>. 
> Then you can get rid of your *_SHIFT macros.
> 
>> +        } else {
>> +            reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> +            reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT;
>> +        }
>> +    } else if (is_layer_vid(layer)) {
>> +        reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> +        reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT;
>> +    } else {
>> +        reg &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> +        reg |= mode << ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT;
>> +    }
> 
> Empty line here (spacing after block)
> 
>> +    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, reg);
>>  }
>>
>>  /**
>> - * zynqmp_disp_avbuf_disable_audio - Disable audio
>> + * zynqmp_disp_avbuf_enable_audio - Enable audio
>>   * @avbuf: Audio/video buffer manager
>>   *
>> - * Disable all audio buffers.
>> + * Enable all audio buffers.
>>   */
>> -static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf 
>> *avbuf)
>> +static void zynqmp_disp_avbuf_enable_audio(struct zynqmp_disp_avbuf 
>> *avbuf)
>>  {
>> -    u32 val;
>> -
>> -    val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -    val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK;
>> -    val |= ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE;
> 
> Same as above with FIELD_PREP().
> 
>> -    val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN;
>> -    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +    zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_MEM);
>> +    zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_ENABLE);
>>  }
>>
>>  /**
>> - * zynqmp_disp_avbuf_enable_video - Enable a video layer
>> + * zynqmp_disp_avbuf_disable_audio - Disable audio
>>   * @avbuf: Audio/video buffer manager
>> - * @layer: The layer
>> - * @mode: Operating mode of layer
>>   *
>> - * Enable the video/graphics buffer for @layer.
>> + * Disable all audio buffers.
>>   */
>> -static void zynqmp_disp_avbuf_enable_video(struct zynqmp_disp_avbuf 
>> *avbuf,
>> -                       struct zynqmp_disp_layer *layer,
>> -                       enum zynqmp_disp_layer_mode mode)
>> +static void zynqmp_disp_avbuf_disable_audio(struct zynqmp_disp_avbuf 
>> *avbuf)
>>  {
>> -    u32 val;
>> -
>> -    val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -    if (is_layer_vid(layer)) {
>> -        val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> -        if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>> -            val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM;
>> -        else
>> -            val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE;
>> -    } else {
>> -        val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> -        val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
>> -        if (mode == ZYNQMP_DISP_LAYER_NONLIVE)
>> -            val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM;
>> -        else
>> -            val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE;
>> -    }
>> -    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
>> +    zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD1_MODE_DISABLE);
>> +    zynqmp_disp_avbuf_output_select(avbuf, NULL, AUD2_MODE_DISABLE);
>>  }
>>
>>  /**
>> - * zynqmp_disp_avbuf_disable_video - Disable a video layer
>> - * @avbuf: Audio/video buffer manager
>> + * zynqmp_disp_avbuf_set_layer_output -Set layer output
> 
> You're missing a space after the dash character.
> 
>>   * @layer: The layer
>> + * @mode: The layer mode
>>   *
>> - * Disable the video/graphics buffer for @layer.
>> + * Set output for @layer
>>   */
>> -static void zynqmp_disp_avbuf_disable_video(struct zynqmp_disp_avbuf 
>> *avbuf,
>> -                        struct zynqmp_disp_layer *layer)
>> +static void zynqmp_disp_avbuf_set_layer_output(struct 
>> zynqmp_disp_layer *layer,
>> +                       enum zynqmp_disp_layer_mode mode)
>>  {
>> -    u32 val;
>> -
>> -    val = zynqmp_disp_avbuf_read(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT);
>> -    if (is_layer_vid(layer)) {
>> -        val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK;
>> -        val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE;
>> -    } else {
>> -        val &= ~ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK;
>> -        val |= ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE;
>> +    int val;
>> +    struct zynqmp_disp *disp = layer->disp;
> 
> I'd swap these two lines above - variables are usually defined in 
> "reverse christmas tree" order, the longest line first, the smallest 
> line last. No big deal though.
> 
>> +
>> +    switch (mode) {
>> +    case ZYNQMP_DISP_LAYER_LIVE:
>> +        val = is_layer_vid(layer) ? VID_MODE_LIVE : GFX_MODE_LIVE;
>> +        break;
>> +    case ZYNQMP_DISP_LAYER_MEM:
>> +        val = is_layer_vid(layer) ? VID_MODE_MEM : GFX_MODE_MEM;
>> +        break;
>> +    case ZYNQMP_DISP_LAYER_TPG:
>> +        if (!is_layer_vid(layer)) {
>> +            dev_err(disp->dev, "gfx layer has no tpg mode\n");
>> +            return;
>> +        }
>> +        val = VID_MODE_TPG;
>> +        break;
>> +    case ZYNQMP_DISP_LAYER_DISABLE:
>> +        val = is_layer_vid(layer) ? VID_MODE_NONE : GFX_MODE_DISABLE;
>> +        break;
>> +    default:
>> +        dev_err(disp->dev, "invalid layer mode\n");
>> +        return;
>>      }
>> -    zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_OUTPUT, val);
> 
> While you're at it, you can add an empty line here (spacing after block)
> 
>> +    zynqmp_disp_avbuf_output_select(&disp->avbuf, layer, val);
>>  }
>>
>>  /**
>> @@ -1030,11 +1063,10 @@ zynqmp_disp_layer_find_format(struct 
>> zynqmp_disp_layer *layer,
>>   */
>>  static void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
>>  {
>> -    zynqmp_disp_avbuf_enable_video(&layer->disp->avbuf, layer,
>> -                       ZYNQMP_DISP_LAYER_NONLIVE);
>> +    zynqmp_disp_avbuf_set_layer_output(layer, ZYNQMP_DISP_LAYER_MEM);
>>      zynqmp_disp_blend_layer_enable(&layer->disp->blend, layer);
>>
>> -    layer->mode = ZYNQMP_DISP_LAYER_NONLIVE;
>> +    layer->mode = ZYNQMP_DISP_LAYER_MEM;
>>  }
>>
>>  /**
>> @@ -1051,7 +1083,7 @@ static void zynqmp_disp_layer_disable(struct 
>> zynqmp_disp_layer *layer)
>>      for (i = 0; i < layer->drm_fmt->num_planes; i++)
>>          dmaengine_terminate_sync(layer->dmas[i].chan);
>>
>> -    zynqmp_disp_avbuf_disable_video(&layer->disp->avbuf, layer);
>> +    zynqmp_disp_avbuf_set_layer_output(layer, 
>> ZYNQMP_DISP_LAYER_DISABLE);
>>      zynqmp_disp_blend_layer_disable(&layer->disp->blend, layer);
>>  }
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h 
>> b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> index f92a006d5070..dad3e356d9ab 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
>> @@ -120,23 +120,12 @@
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT            0x70
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_SHIFT        0
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MASK        (0x3 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_LIVE        (0 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_MEM        (1 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_PATTERN        (2 << 0)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID1_NONE        (3 << 0)
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_SHIFT        2
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MASK        (0x3 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_DISABLE        (0 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_MEM        (1 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_LIVE        (2 << 2)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_VID2_NONE        (3 << 2)
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_SHIFT        4
>>  #define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MASK        (0x3 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PL        (0 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_MEM        (1 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_PATTERN        (2 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD1_DISABLE        (3 << 4)
>> -#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_EN        BIT(6)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_MASK        (0x1 << 6)
>> +#define ZYNQMP_DISP_AV_BUF_OUTPUT_AUD2_SHIFT        6
> 
> Please use BIT() or GENMASK(). You don't need the _SHIFT macros if you 
> use FIELD_PREP() / FIELD_GET().
> 
> Cheers,
> -Paul
> 
>>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT0        0x74
>>  #define ZYNQMP_DISP_AV_BUF_HCOUNT_VCOUNT_INT1        0x78
>>  #define ZYNQMP_DISP_AV_BUF_PATTERN_GEN_SELECT        0x100
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel